On Tue, Mar 25, 2014 at 2:35 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> A datapath pointer is available only when holding a lock. Change
> ovs_flow_cmd_fill_info() and ovs_flow_cmd_build_info() to take a
> dp_ifindex directly, rather than a datapath pointer that is then
> (only) used to get the dp_ifindex. This is useful, since the
> dp_ifindex is available even when the datapath pointer is not, both
> before and after taking a lock, which makes further critical section
> reduction possible.
>
After looking at this patch and last one, it looks like we are doing
log more work just to make RCU checker happy.
Can we just use rcu_read_lock() and unlock() to protect acts de-referencing ?
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
> ---
> v5: Split to a separate patch, relaxed locking requirements.
>
> datapath/datapath.c | 58
> ++++++++++++++++++++++++++++++++-------------------
> datapath/flow.c | 6 ++++--
> datapath/flow.h | 2 +-
> 3 files changed, 41 insertions(+), 25 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index d1b0155..01913f6 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -663,8 +663,17 @@ static size_t ovs_flow_cmd_msg_size(const struct
> sw_flow_actions *acts)
> + nla_total_size(acts->actions_len); /* OVS_FLOW_ATTR_ACTIONS
> */
> }
>
> -/* Called with ovs_mutex or RCU read lock. */
> -static int ovs_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
> +/* Allow a flow to be accessed if:
> + * - RCU read lock is held, or
> + * - OVS mutex is held, or
> + * - the flow has already been removed from the flow table. */
> +#define ovsl_dereference_flow_acts(flow) \
> + rcu_dereference_check(flow->sf_acts, (lockdep_ovsl_is_held() \
> + || ovs_flow_removed(flow)))
> +
> +/* Must be called with rcu_read_lock or ovs_mutex if 'flow' is in the flow
> + * table. */
> +static int ovs_flow_cmd_fill_info(const struct sw_flow *flow, int dp_ifindex,
> struct sk_buff *skb, u32 portid,
> u32 seq, u32 flags, u8 cmd)
> {
> @@ -681,7 +690,7 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow,
> struct datapath *dp,
> if (!ovs_header)
> return -EMSGSIZE;
>
> - ovs_header->dp_ifindex = get_dpifindex(dp);
> + ovs_header->dp_ifindex = dp_ifindex;
>
> /* Fill flow key. */
> nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
> @@ -730,8 +739,7 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow,
> struct datapath *dp,
> if (start) {
> const struct sw_flow_actions *sf_acts;
>
> - sf_acts = rcu_dereference_ovsl(flow->sf_acts);
> -
> + sf_acts = ovsl_dereference_flow_acts(flow);
> err = ovs_nla_put_actions(sf_acts->actions,
> sf_acts->actions_len, skb);
> if (!err)
> @@ -754,8 +762,9 @@ error:
> return err;
> }
>
> -/* Must be called with ovs_mutex. */
> -static struct sk_buff *ovs_flow_cmd_alloc_info(struct sw_flow *flow,
> +/* Must be called with rcu_read_lock or ovs_mutex if 'flow' is in the flow
> + * table. */
> +static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow *flow,
> struct genl_info *info,
> bool always)
> {
> @@ -765,7 +774,7 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(struct
> sw_flow *flow,
> if (!always && !ovs_must_notify(info, &ovs_dp_flow_multicast_group))
> return NULL;
>
> - len = ovs_flow_cmd_msg_size(ovsl_dereference(flow->sf_acts));
> + len = ovs_flow_cmd_msg_size(ovsl_dereference_flow_acts(flow));
>
> skb = genlmsg_new_unicast(len, info, GFP_KERNEL);
>
> @@ -775,11 +784,12 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(struct
> sw_flow *flow,
> return skb;
> }
>
> -/* Must be called with ovs_mutex. */
> -static struct sk_buff *ovs_flow_cmd_build_info(struct sw_flow *flow,
> - struct datapath *dp,
> - struct genl_info *info,
> - u8 cmd, bool always)
> +/* Must be called with rcu_read_lock or ovs_mutex if 'flow' is in the flow
> + * table. */
> +static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow,
> + int dp_ifindex,
> + struct genl_info *info, u8 cmd,
> + bool always)
> {
> struct sk_buff *skb;
> int retval;
> @@ -788,8 +798,9 @@ static struct sk_buff *ovs_flow_cmd_build_info(struct
> sw_flow *flow,
> if (!skb || IS_ERR(skb))
> return skb;
>
> - retval = ovs_flow_cmd_fill_info(flow, dp, skb, info->snd_portid,
> - info->snd_seq, 0, cmd);
> + retval = ovs_flow_cmd_fill_info(flow, dp_ifindex, skb,
> + info->snd_portid, info->snd_seq, 0,
> + cmd);
> BUG_ON(retval < 0);
> return skb;
> }
> @@ -870,8 +881,8 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
> struct genl_info *info)
> goto err_flow_free;
> }
>
> - reply = ovs_flow_cmd_build_info(flow, dp, info,
> - OVS_FLOW_CMD_NEW, false);
> + reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
> + info, OVS_FLOW_CMD_NEW,
> false);
> } else {
> /* We found a matching flow. */
> /* Bail out if we're not allowed to modify an existing flow.
> @@ -897,8 +908,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
> struct genl_info *info)
> rcu_assign_pointer(flow->sf_acts, acts);
> ovs_nla_free_flow_actions(old_acts);
> }
> - reply = ovs_flow_cmd_build_info(flow, dp, info,
> - OVS_FLOW_CMD_NEW, false);
> +
> + reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
> + info, OVS_FLOW_CMD_NEW,
> false);
>
> /* Clear stats. */
> if (a[OVS_FLOW_ATTR_CLEAR])
> @@ -960,7 +972,8 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct
> genl_info *info)
> goto unlock;
> }
>
> - reply = ovs_flow_cmd_build_info(flow, dp, info, OVS_FLOW_CMD_NEW,
> true);
> + reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex, info,
> + OVS_FLOW_CMD_NEW, true);
> if (IS_ERR(reply)) {
> err = PTR_ERR(reply);
> goto unlock;
> @@ -1016,7 +1029,8 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct
> genl_info *info)
> ovs_flow_tbl_remove(&dp->table, flow);
>
> if (reply) {
> - err = ovs_flow_cmd_fill_info(flow, dp, reply,
> info->snd_portid,
> + err = ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex,
> + reply, info->snd_portid,
> info->snd_seq, 0,
> OVS_FLOW_CMD_DEL);
> BUG_ON(err < 0);
> @@ -1057,7 +1071,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb,
> struct netlink_callback *cb)
> if (!flow)
> break;
>
> - if (ovs_flow_cmd_fill_info(flow, dp, skb,
> + if (ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex, skb,
> NETLINK_CB(cb->skb).portid,
> cb->nlh->nlmsg_seq, NLM_F_MULTI,
> OVS_FLOW_CMD_NEW) < 0)
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 5d1d2f2..6d7c3b5 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -123,8 +123,10 @@ unlock:
> spin_unlock(&stats->lock);
> }
>
> -/* Called with ovs_mutex. */
> -void ovs_flow_stats_get(struct sw_flow *flow, struct ovs_flow_stats
> *ovs_stats,
> +/* Must be called with rcu_read_lock or ovs_mutex if 'flow' is in the flow
> + * table. */
> +void ovs_flow_stats_get(const struct sw_flow *flow,
> + struct ovs_flow_stats *ovs_stats,
> unsigned long *used, __be16 *tcp_flags)
> {
> int node;
> diff --git a/datapath/flow.h b/datapath/flow.h
> index 5587577..25eba04 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -183,7 +183,7 @@ struct arp_eth_header {
> } __packed;
>
> void ovs_flow_stats_update(struct sw_flow *flow, struct sk_buff *skb);
> -void ovs_flow_stats_get(struct sw_flow *flow, struct ovs_flow_stats *stats,
> +void ovs_flow_stats_get(const struct sw_flow *, struct ovs_flow_stats *,
> unsigned long *used, __be16 *tcp_flags);
> void ovs_flow_stats_clear(struct sw_flow *flow);
> u64 ovs_flow_used_time(unsigned long flow_jiffies);
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
