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

[Jakub Kicinski <kuba@kernel.org>]

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.