On Thu, Sep 21, 2017 at 04:43:02PM -0700, Cong Wang wrote:
> Cc: Chris Mi <chr...@mellanox.com>
> Cc: Jamal Hadi Salim <j...@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
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.
> ---
> 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.
> + }
> ht->tp_c = tp_c;
> ht->refcnt = 1;
> ht->divisor = divisor;
> ht->handle = handle;
> ht->prio = tp->prio;
> + idr_init(&ht->handle_idr);
>
> err = u32_replace_hw_hnode(tp, ht, flags);
> if (err) {
> + idr_remove_ext(&tp_c->handle_idr, handle);
> kfree(ht);
> return err;
> }
...
