| Message ID | 1524113961-30166-1-git-send-email-lirongqing@baidu.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: net_cls: remove a NULL check for css_cls_state | expand |
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.
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. >
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.
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; }
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(-)