From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pravin Shelar Subject: Re: [net-next v2 3/4] openvswitch: Add meter infrastructure Date: Fri, 20 Oct 2017 20:32:44 -0700 Message-ID: References: <1508225805-11613-1-git-send-email-azhou@ovn.org> <1508225805-11613-4-git-send-email-azhou@ovn.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Greg Rose , Joe Stringer , Linux Kernel Network Developers To: Andy Zhou Return-path: Received: from relay3-d.mail.gandi.net ([217.70.183.195]:42545 "EHLO relay3-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752664AbdJUDcq (ORCPT ); Fri, 20 Oct 2017 23:32:46 -0400 Received: from mail-wr0-f172.google.com (mail-wr0-f172.google.com [209.85.128.172]) (Authenticated sender: pshelar@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 2A56AA80C1 for ; Sat, 21 Oct 2017 05:32:45 +0200 (CEST) Received: by mail-wr0-f172.google.com with SMTP id g90so12785515wrd.6 for ; Fri, 20 Oct 2017 20:32:45 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Oct 19, 2017 at 5:58 PM, Andy Zhou wrote: > > On Thu, Oct 19, 2017 at 02:47 Pravin Shelar wrote: >> >> On Tue, Oct 17, 2017 at 12:36 AM, Andy Zhou 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 >> > --- >> > 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.