On Thu, Oct 19, 2017 at 5:58 PM, Andy Zhou <az...@ovn.org> wrote: > > On Thu, Oct 19, 2017 at 02:47 Pravin Shelar <pshe...@ovn.org> wrote: >> >> On Tue, Oct 17, 2017 at 12:36 AM, Andy Zhou <az...@ovn.org> wrote: >> > OVS kernel datapath so far does not support Openflow meter action. >> > This is the first stab at adding kernel datapath meter support. >> > This implementation supports only drop band type. >> > >> > Signed-off-by: Andy Zhou <az...@ovn.org> >> > --- >> > net/openvswitch/Makefile | 1 + >> > net/openvswitch/datapath.c | 14 +- >> > net/openvswitch/datapath.h | 3 + >> > net/openvswitch/meter.c | 604 >> > +++++++++++++++++++++++++++++++++++++++++++++ >> > net/openvswitch/meter.h | 54 ++++ >> > 5 files changed, 674 insertions(+), 2 deletions(-) >> > create mode 100644 net/openvswitch/meter.c >> > create mode 100644 net/openvswitch/meter.h >> > >> This patch mostly looks good. I have one comment below. >> >> > +static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info >> > *info) >> > +{ >> > + struct nlattr **a = info->attrs; >> > + struct dp_meter *meter, *old_meter; >> > + struct sk_buff *reply; >> > + struct ovs_header *ovs_reply_header; >> > + struct ovs_header *ovs_header = info->userhdr; >> > + struct datapath *dp; >> > + int err; >> > + u32 meter_id; >> > + bool failed; >> > + >> > + meter = dp_meter_create(a); >> > + if (IS_ERR_OR_NULL(meter)) >> > + return PTR_ERR(meter); >> > + >> > + reply = ovs_meter_cmd_reply_start(info, OVS_METER_CMD_SET, >> > + &ovs_reply_header); >> > + if (IS_ERR(reply)) { >> > + err = PTR_ERR(reply); >> > + goto exit_free_meter; >> > + } >> > + >> > + ovs_lock(); >> > + dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex); >> > + if (!dp) { >> > + err = -ENODEV; >> > + goto exit_unlock; >> > + } >> > + >> > + if (!a[OVS_METER_ATTR_ID]) { >> > + err = -ENODEV; >> > + goto exit_unlock; >> > + } >> > + >> > + meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]); >> > + >> > + /* Cannot fail after this. */ >> > + old_meter = lookup_meter(dp, meter_id); >> I do not see RCU read lock taken here. This is not correctness issue >> but it could cause RCU checker to spit out warning message. You could >> do same trick that is done in get_dp() to avoid this issue. > > O.K. >> >> >> >> Can you also test the code with rcu sparse check config option enabled? > > > Do you mean to sparse compile with CONFIG_PROVE_LOCKING and > CONFIG_DENUG_OBJECTS_RCU_HEAD?
You could use all following options simultaneously: CONFIG_PREEMPT CONFIG_DEBUG_PREEMPT CONFIG_DEBUG_SPINLOCK CONFIG_DEBUG_ATOMIC_SLEEP CONFIG_PROVE_RCU CONFIG_DEBUG_OBJECTS_RCU_HEAD Thanks, Pravin.