netdev - Re: [patch net-next 2/3] net/sched: Change cls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170830103006.GB14786@vergenet.net>
Date:   Wed, 30 Aug 2017 12:30:07 +0200
From:   Simon Horman <simon.horman@...ronome.com>
To:     Chris Mi <chrism@...lanox.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "mawilcox@...rosoft.com" <mawilcox@...rosoft.com>
Subject: Re: [patch net-next 2/3] net/sched: Change cls_flower to use IDR

On Tue, Aug 29, 2017 at 03:25:35AM +0000, Chris Mi wrote:
> 
> 
> > -----Original Message-----
> > From: Simon Horman [mailto:simon.horman@...ronome.com]
> > Sent: Monday, August 28, 2017 7:37 PM
> > To: Chris Mi <chrism@...lanox.com>
> > Cc: netdev@...r.kernel.org; jhs@...atatu.com;
> > xiyou.wangcong@...il.com; jiri@...nulli.us; davem@...emloft.net;
> > mawilcox@...rosoft.com
> > Subject: Re: [patch net-next 2/3] net/sched: Change cls_flower to use IDR
> > 
> > On Mon, Aug 28, 2017 at 02:41:16AM -0400, Chris Mi wrote:
> > > Currently, all filters with the same priority are linked in a doubly
> > > linked list. Every filter should have a unique handle. To make the
> > > handle unique, we need to iterate the list every time to see if the
> > > handle exists or not when inserting a new filter. It is time-consuming.
> > > For example, it takes about 5m3.169s to insert 64K rules.
> > >
> > > This patch changes cls_flower to use IDR. With this patch, it takes
> > > about 0m1.127s to insert 64K rules. The improvement is huge.
> > 
> > Very nice :)
> > 
> > > But please note that in this testing, all filters share the same action.
> > > If every filter has a unique action, that is another bottleneck.
> > > Follow-up patch in this patchset addresses that.
> > >
> > > Signed-off-by: Chris Mi <chrism@...lanox.com>
> > > Signed-off-by: Jiri Pirko <jiri@...lanox.com>
> > > ---
> > >  net/sched/cls_flower.c | 55
> > > +++++++++++++++++++++-----------------------------
> > >  1 file changed, 23 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index
> > > bd9dab4..3d041d2 100644
> > > --- a/net/sched/cls_flower.c
> > > +++ b/net/sched/cls_flower.c
> > 
> > ...
> > 
> > > @@ -890,6 +870,7 @@ static int fl_change(struct net *net, struct sk_buff
> > *in_skb,
> > >  	struct cls_fl_filter *fnew;
> > >  	struct nlattr **tb;
> > >  	struct fl_flow_mask mask = {};
> > > +	unsigned long idr_index;
> > >  	int err;
> > >
> > >  	if (!tca[TCA_OPTIONS])
> > > @@ -920,13 +901,21 @@ static int fl_change(struct net *net, struct sk_buff
> > *in_skb,
> > >  		goto errout;
> > >
> > >  	if (!handle) {
> > > -		handle = fl_grab_new_handle(tp, head);
> > > -		if (!handle) {
> > > -			err = -EINVAL;
> > > +		err = idr_alloc_ext(&head->handle_idr, fnew, &idr_index,
> > > +				    1, 0x80000000, GFP_KERNEL);
> > > +		if (err)
> > >  			goto errout;
> > > -		}
> > > +		fnew->handle = idr_index;
> > > +	}
> > > +
> > > +	/* user specifies a handle and it doesn't exist */
> > > +	if (handle && !fold) {
> > > +		err = idr_alloc_ext(&head->handle_idr, fnew, &idr_index,
> > > +				    handle, handle + 1, GFP_KERNEL);
> > > +		if (err)
> > > +			goto errout;
> > > +		fnew->handle = idr_index;
> > >  	}
> > > -	fnew->handle = handle;
> > >
> > >  	if (tb[TCA_FLOWER_FLAGS]) {
> > >  		fnew->flags = nla_get_u32(tb[TCA_FLOWER_FLAGS]);
> > > @@ -980,6 +969,8 @@ static int fl_change(struct net *net, struct sk_buff
> > *in_skb,
> > >  	*arg = fnew;
> > >
> > >  	if (fold) {
> > > +		fnew->handle = handle;
> > 
> > Can it be the case that fold is non-NULL and handle is zero?
> > The handling of that case seem to have changed in this patch.
> I don't think that could happen.  In function tc_ctl_tfilter(),
> 
> fl_get() will be called.  If handle is zero, fl_get() will return NULL.
> That means fold is NULL.

Thanks for the explanation, I see that now.

> > > +		idr_replace_ext(&head->handle_idr, fnew, fnew->handle);
> > >  		list_replace_rcu(&fold->list, &fnew->list);
> > >  		tcf_unbind_filter(tp, &fold->res);
> > >  		call_rcu(&fold->rcu, fl_destroy_filter);