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, 3 Nov 2017 09:22:48 -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]:35107 "EHLO relay3-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753059AbdKCQWv (ORCPT ); Fri, 3 Nov 2017 12:22:51 -0400 Received: from mail-wr0-f173.google.com (mail-wr0-f173.google.com [209.85.128.173]) (Authenticated sender: pshelar@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 820BFA80ED for ; Fri, 3 Nov 2017 17:22:49 +0100 (CET) Received: by mail-wr0-f173.google.com with SMTP id o44so2960461wrf.11 for ; Fri, 03 Nov 2017 09:22:49 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Nov 2, 2017 at 7:43 PM, Andy Zhou wrote: > On Thu, Nov 2, 2017 at 5:07 AM, Pravin Shelar wrote: >> On Thu, Nov 2, 2017 at 3:07 AM, Andy Zhou wrote: >>> On Fri, Oct 20, 2017 at 8:32 PM, Pravin Shelar wrote: >>>> 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, I turned on those flags but did not get any error message. Do you >>> mind share the RCU checker message? >> >> There would be assert failure and stack trace. so it would be pretty >> obvious in kernel log messages. >> Let me know if you do not see any stack trace while running meter >> create, delete and execute. > > No I did not see them. ok, Can you send out patch against latest net-next?