Re: [Patch net-next] net_sched: use idr to allocate u32 filter handles

[Simon Horman <simon.horman@netronome.com>]

On Thu, Sep 28, 2017 at 03:19:05PM -0700, Cong Wang wrote:
> On Thu, Sep 28, 2017 at 12:34 AM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > Hi Cong,
> >
> > this looks like a nice enhancement to me. Did you measure any performance
> > benefit from it.  Perhaps it could be described in the changelog_ I also
> > have a more detailed question below.
> 
> No, I am inspired by commit c15ab236d69d, don't measure it.

Perhaps it would be nice to note that in the changelog.

> >> ---
> >>  net/sched/cls_u32.c | 108 ++++++++++++++++++++++++++++++++--------------------
> >>  1 file changed, 67 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> >> index 10b8d851fc6b..316b8a791b13 100644
> >> --- a/net/sched/cls_u32.c
> >> +++ b/net/sched/cls_u32.c
> >> @@ -46,6 +46,7 @@
> >
> > ...
> >
> >> @@ -937,22 +940,33 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
> >>                       return -EINVAL;
> >>               if (TC_U32_KEY(handle))
> >>                       return -EINVAL;
> >> -             if (handle == 0) {
> >> -                     handle = gen_new_htid(tp->data);
> >> -                     if (handle == 0)
> >> -                             return -ENOMEM;
> >> -             }
> >>               ht = kzalloc(sizeof(*ht) + divisor*sizeof(void *), GFP_KERNEL);
> >>               if (ht == NULL)
> >>                       return -ENOBUFS;
> >> +             if (handle == 0) {
> >> +                     handle = gen_new_htid(tp->data, ht);
> >> +                     if (handle == 0) {
> >> +                             kfree(ht);
> >> +                             return -ENOMEM;
> >> +                     }
> >> +             } else {
> >> +                     err = idr_alloc_ext(&tp_c->handle_idr, ht, NULL,
> >> +                                         handle, handle + 1, GFP_KERNEL);
> >> +                     if (err) {
> >> +                             kfree(ht);
> >> +                             return err;
> >> +                     }
> >
> > The above seems to check that handle is not already in use and mark it as
> > in use. But I don't see that logic in the code prior to this patch.
> > Am I missing something? If not perhaps this portion should be a separate
> > patch or described in the changelog.
> 
> The logic is in upper layer, tc_ctl_tfilter(). It tries to get a
> filter by handle
> (if non-zero), and errors out if we are creating a new filter with the same
> handle.
> 
> At the point you quote above, 'n' is already NULL and 'handle' is non-zero,
> which means there is no existing filter has same handle, it is safe to just
> mark it as in-use.

Thanks for the clarification, that seems fine to me.

Reviewed-by: Simon Horman <simon.horman@netronome.com>