From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH net-next v4 1/2] net sched actions: Add support for user cookies Date: Tue, 17 Jan 2017 18:05:01 +0100 Message-ID: <20170117170501.GE1807@nanopsycho.orion> References: <1484651509-27500-1-git-send-email-jhs@emojatatu.com> <1484651509-27500-2-git-send-email-jhs@emojatatu.com> <587E2747.9000207@iogearbox.net> <587E4D16.8030708@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jamal Hadi Salim , davem@davemloft.net, netdev@vger.kernel.org, jiri@mellanox.com, paulb@mellanox.com, john.fastabend@gmail.com, simon.horman@netronome.com, mrv@mojatatu.com, hadarh@mellanox.com, ogerlitz@mellanox.com, roid@mellanox.com, xiyou.wangcong@gmail.com To: Daniel Borkmann Return-path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:34236 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750830AbdAQR3M (ORCPT ); Tue, 17 Jan 2017 12:29:12 -0500 Received: by mail-wm0-f67.google.com with SMTP id c85so7510276wmi.1 for ; Tue, 17 Jan 2017 09:28:22 -0800 (PST) Content-Disposition: inline In-Reply-To: <587E4D16.8030708@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: Tue, Jan 17, 2017 at 05:57:58PM CET, daniel@iogearbox.net wrote: >On 01/17/2017 05:50 PM, Jamal Hadi Salim wrote: >> On 17-01-17 09:16 AM, Daniel Borkmann wrote: >> > On 01/17/2017 12:11 PM, Jamal Hadi Salim wrote: >> > > From: Jamal Hadi Salim >> > > >> > > Introduce optional 128-bit action cookie. >> > > Like all other cookie schemes in the networking world (eg in protocols >> > > like http or existing kernel fib protocol field, etc) the idea is to save >> > > user state that when retrieved serves as a correlator. The kernel >> > > _should not_ intepret it. The user can store whatever they wish in the >> > > 128 bits. >> > [...] >> > >> > Since it looks like you need a v5 anyway, few comments below. >> > >> > > include/net/act_api.h | 1 + >> > > include/net/pkt_cls.h | 8 ++++++++ >> > > include/uapi/linux/pkt_cls.h | 3 +++ >> > > net/sched/act_api.c | 25 +++++++++++++++++++++++++ >> > > 4 files changed, 37 insertions(+) >> > > >> > > diff --git a/include/net/act_api.h b/include/net/act_api.h >> > > index 1d71644..0692458 100644 >> > > --- a/include/net/act_api.h >> > > +++ b/include/net/act_api.h >> > > @@ -41,6 +41,7 @@ struct tc_action { >> > > struct rcu_head tcfa_rcu; >> > > struct gnet_stats_basic_cpu __percpu *cpu_bstats; >> > > struct gnet_stats_queue __percpu *cpu_qstats; >> > > + struct tc_cookie *act_ck; >> > >> > Since we know anyway that this is part of struct tc_action, can't >> > you just give this some real/readable name like ... >> > >> > struct tc_cookie cookie; >> >> Grep-ability. >> I was worried about when the classifier adds its cookie it >> would need to use something like cls_cookie etc. > >Given this cookie is just used for correlation in user space anyway >and not processed any further by the kernel, I think we can well >handle these very few spots, so would be better if the code is more >maintainable instead. I agree with Daniel. His naming change suggestions make sense. In fact, Jamal, now I know why there are names like this all over TC :)