On Thu, Sep 28, 2017 at 12:34 AM, Simon Horman
[off-list ref] 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.

quoted

---
 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 @@

...

quoted

@@ -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.