On Tue, Jul 02, 2024 at 11:53:01AM +0200, Ilya Maximets wrote: > On 7/2/24 11:37, Simon Horman wrote: > > On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote: > >> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote: > >>> Adrian Moreno <[email protected]> writes: > > > > ... > > > >>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > > > > ... > > > >>>> @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, > >>>> struct sw_flow_key *key) > >>>> return 0; > >>>> } > >>>> > >>>> +#if IS_ENABLED(CONFIG_PSAMPLE) > >>>> +static void execute_psample(struct datapath *dp, struct sk_buff *skb, > >>>> + const struct nlattr *attr) > >>>> +{ > >>>> + struct psample_group psample_group = {}; > >>>> + struct psample_metadata md = {}; > >>>> + const struct nlattr *a; > >>>> + int rem; > >>>> + > >>>> + nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) { > >>>> + switch (nla_type(a)) { > >>>> + case OVS_PSAMPLE_ATTR_GROUP: > >>>> + psample_group.group_num = nla_get_u32(a); > >>>> + break; > >>>> + > >>>> + case OVS_PSAMPLE_ATTR_COOKIE: > >>>> + md.user_cookie = nla_data(a); > >>>> + md.user_cookie_len = nla_len(a); > >>>> + break; > >>>> + } > >>>> + } > >>>> + > >>>> + psample_group.net = ovs_dp_get_net(dp); > >>>> + md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex; > >>>> + md.trunc_size = skb->len - OVS_CB(skb)->cutlen; > >>>> + > >>>> + psample_sample_packet(&psample_group, skb, 0, &md); > >>>> +} > >>>> +#else > >>>> +static inline void execute_psample(struct datapath *dp, struct sk_buff > >>>> *skb, > >>>> + const struct nlattr *attr) {} > >>> > >>> I noticed that this got flagged in patchwork since it is 'static inline' > >>> while being part of a complete translation unit - but I also see some > >>> other places where that has been done. I guess it should be just > >>> 'static' though. I don't feel very strongly about it. > >>> > >> > >> We had a bit of discussion about this with Ilya. It seems "static > >> inline" is a common pattern around the kernel. The coding style > >> documentation says: > >> "Generally, inline functions are preferable to macros resembling > >> functions." > >> > >> So I think this "inline" is correct but I might be missing something. > > > > Hi Adrián, > > > > TL;DR: Please remove this inline keyword > > > > For Kernel networking code at least it is strongly preferred not > > to use inline in .c files unless there is a demonstrable - usually > > performance - reason to do so. Rather, it is preferred to let the > > compiler decide when to inline such functions. OTOH, the inline > > keyword in .h files is fine. > > FWIW, the main reason for 'inline' here is not performance, but silencing > compiler's potential 'maybe unused' warnings: > > Function-like macros with unused parameters should be replaced by static > inline functions to avoid the issue of unused variables > > I think, the rule for static inline functions in .c files is at odds with > the 'Conditional Compilation' section of coding style. The section does > recommend to avoid conditional function declaration in .c files, but I'm not > sure it is reasonable to export internal static functions for that reason. > > In this particular case we can either define a macro, which is discouraged > by the coding style: > > Generally, inline functions are preferable to macros resembling functions. > > Or create a static inline function, that is against rule of no static > inline functions in .c files. > > Or create a simple static function and mark all the arguments as unused, > which kind of compliant to the coding style, but the least pretty.
Hi Ilya, I guess I would lean towards the last option. But in any case, thanks for pointing out that this is complex: I had not realised that when I wrote my previous email. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
