From: Simon Horman <simon.horman@netronome•com>
To: Cong Wang <xiyou.wangcong@gmail•com>
Cc: Linux Kernel Network Developers <netdev@vger•kernel.org>,
Chris Mi <chrism@mellanox•com>,
Jamal Hadi Salim <jhs@mojatatu•com>
Subject: Re: [Patch net-next] net_sched: use idr to allocate u32 filter handles
Date: Fri, 29 Sep 2017 08:46:01 +0200 [thread overview]
Message-ID: <20170929064559.GA7342@netronome.com> (raw)
In-Reply-To: <CAM_iQpWbiA4QZb8j8fau7kMhzeLpRppEMcmB80byqTUryPheOw@mail.gmail.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>
prev parent reply other threads:[~2017-09-29 6:46 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-21 23:43 [Patch net-next] net_sched: use idr to allocate u32 filter handles Cong Wang
2017-09-28 7:34 ` Simon Horman
2017-09-28 22:19 ` Cong Wang
2017-09-29 6:46 ` Simon Horman [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170929064559.GA7342@netronome.com \
--to=simon.horman@netronome$(echo .)com \
--cc=chrism@mellanox$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox