netdev - Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221201143812.47089fb1@kernel.org>
Date:   Thu, 1 Dec 2022 14:38:12 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Pedro Tammela <pctammela@...atatu.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com, jhs@...atatu.com, xiyou.wangcong@...il.com,
        jiri@...nulli.us, kuniyu@...zon.com
Subject: Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc

On Thu, 1 Dec 2022 13:40:34 -0300 Pedro Tammela wrote:
> >> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
> >> +			   struct tcf_result *res)
> >> +{
> >> +	if (0) { /* noop */ }
> >> +#if IS_BUILTIN(CONFIG_NET_ACT_BPF)
> >> +	else if (a->ops->act == tcf_bpf_act)
> >> +		return tcf_bpf_act(skb, a, res);
> >> +#endif  
> > 
> > How does the 'else if' ladder compare to a switch statement?  
> 
> It's the semantically the same, we would just need to do some casts to 
> unsigned long.

Sorry, should've been clearer, I mean in terms of generated code.
Is the machine code identical / better / worse?

> WDYT about the following?
> 
>    #define __TC_ACT_BUILTIN(builtin, fname) \
>       if (builtin && a->ops->act == fname) return fname(skb, a, res)
> 
>    #define _TC_ACT_BUILTIN(builtin, fname) __TC_ACT_BUILTIN(builtin, fname)
>    #define TC_ACT_BUILTIN(cfg, fname)  _TC_ACT_BUILTIN(IS_BUILTIN(cfg), 
> fname)
> 
>    static inline int __tc_act(struct sk_buff *skb, const struct 
> tc_action *a,
>                               struct tcf_result *res)
>    {
>            TC_ACT_BUILTIN(CONFIG_NET_ACT_BPF, tcf_bpf_act);
>    ...
> 
> It might be more pleasant to the reader.

Most definitely not to this reader :) The less macro magic the better.
I'm primarily curious about whether the compiler treats this sort of
construct the same as a switch.

> >> +#ifdef CONFIG_NET_CLS_ACT
> >> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
> >> +			   struct tcf_result *res)
> >> +{
> >> +	return a->ops->act(skb, a, res);
> >> +}
> >> +#endif
> >> +
> >> +#ifdef CONFIG_NET_CLS
> >> +static inline int __tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
> >> +				struct tcf_result *res)
> >> +{
> >> +	return tp->classify(skb, tp, res);
> >> +}
> >> +#endif  
> > 
> > please don't wrap the static inline helpers in #ifdefs unless it's
> > actually necessary for build to pass.  
> 
> The only one really needed is CONFIG_NET_CLS_ACT because the struct 
> tc_action definition is protected by it. Perhaps we should move it out 
> of the #ifdef?

Yes, I think that's a nice cleanup.