[PATCH] net: net_cls: remove a NULL check for css_cls_state

[PATCH] net: net_cls: remove a NULL check for css_cls_state

[Li RongQing]

The input of css_cls_state() is impossible to NULL except
cgrp_css_online, so simplify it

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 net/core/netclassid_cgroup.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/core/netclassid_cgroup.c b/net/core/netclassid_cgroup.c
index 5e4f04004a49..ee087cf793c2 100644
--- a/net/core/netclassid_cgroup.c
+++ b/net/core/netclassid_cgroup.c
@@ -19,7 +19,7 @@
 
 static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state *css)
 {
-	return css ? container_of(css, struct cgroup_cls_state, css) : NULL;
+	return container_of(css, struct cgroup_cls_state, css);
 }
 
 struct cgroup_cls_state *task_cls_state(struct task_struct *p)
@@ -44,10 +44,9 @@ cgrp_css_alloc(struct cgroup_subsys_state *parent_css)
 static int cgrp_css_online(struct cgroup_subsys_state *css)
 {
 	struct cgroup_cls_state *cs = css_cls_state(css);
-	struct cgroup_cls_state *parent = css_cls_state(css->parent);
 
-	if (parent)
-		cs->classid = parent->classid;
+	if (css->parent)
+		cs->classid = css_cls_state(css->parent)->classid;
 
 	return 0;
 }
-- 
2.11.0

Re: [PATCH] net: net_cls: remove a NULL check for css_cls_state

[David Miller]

From: Li RongQing <lirongqing@baidu.com>
Date: Thu, 19 Apr 2018 12:59:21 +0800

> The input of css_cls_state() is impossible to NULL except
> cgrp_css_online, so simplify it
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

I don't view this as an improvement.  Just let the helper always check
NULL and that way there are less situations to audit.

And it's not like this is a critical fast path either.

I'm not applying this, sorry.

Re: [PATCH] net: net_cls: remove a NULL check for css_cls_state

[Li RongQing]

On 4/20/18, David Miller <davem@davemloft.net> wrote:
> From: Li RongQing <lirongqing@baidu.com>
> Date: Thu, 19 Apr 2018 12:59:21 +0800
>
>> The input of css_cls_state() is impossible to NULL except
>> cgrp_css_online, so simplify it
>>
>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
>
> I don't view this as an improvement.  Just let the helper always check
> NULL and that way there are less situations to audit.
>
css_cls_state maybe return NULL, but nearly no places check the return
value with NULL, this seems unreadable.

net/core/netclassid_cgroup.c:27:        return
css_cls_state(task_css_check(p, net_cls_cgrp_id,
net/core/netclassid_cgroup.c:46:        struct cgroup_cls_state *cs =
css_cls_state(css);
net/core/netclassid_cgroup.c:47:        struct cgroup_cls_state
*parent = css_cls_state(css->parent);
net/core/netclassid_cgroup.c:57:        kfree(css_cls_state(css));
net/core/netclassid_cgroup.c:82:                           (void
*)(unsigned long)css_cls_state(css)->classid);
net/core/netclassid_cgroup.c:89:        return css_cls_state(css)->classid;
net/core/netclassid_cgroup.c:95:        struct cgroup_cls_state *cs =
css_cls_state(css);

> And it's not like this is a critical fast path either.
>

I see css_cls_state will be called  when send packet if
CONFIG_NET_CLS_ACT and CONFIG_NET_EGRESS enabled, the calling stack is
like below:

css_cls_state
  task_cls_state
    task_get_classid
       cls_cgroup_classify
          tcf_classify
            sch_handle_egress
               __dev_queue_xmit
                        CONFIG_NET_CLS_ACT
                         CONFIG_NET_EGRESS

-RongQing






> I'm not applying this, sorry.
>

Re: [PATCH] net: net_cls: remove a NULL check for css_cls_state

[David Miller]

From: Li RongQing <roy.qing.li@gmail.com>
Date: Wed, 25 Apr 2018 16:35:02 +0800

> On 4/20/18, David Miller <davem@davemloft.net> wrote:
>> From: Li RongQing <lirongqing@baidu.com>
>> Date: Thu, 19 Apr 2018 12:59:21 +0800
>>
>>> The input of css_cls_state() is impossible to NULL except
>>> cgrp_css_online, so simplify it
>>>
>>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
>>
>> I don't view this as an improvement.  Just let the helper always check
>> NULL and that way there are less situations to audit.
>>
> css_cls_state maybe return NULL, but nearly no places check the return
> value with NULL, this seems unreadable.

I saw this email of your's the first few times, you don't need to post
it again.

I still disagree with your analysis, and I do not see your change as
an overall improvement.

Thank you.