[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240702123301.GH598357@kernel.org>
Date: Tue, 2 Jul 2024 13:33:01 +0100
From: Simon Horman <horms@...nel.org>
To: Ilya Maximets <i.maximets@....org>
Cc: Adrián Moreno <amorenoz@...hat.com>,
dev@...nvswitch.org, Donald Hunter <donald.hunter@...il.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
"David S. Miller" <davem@...emloft.net>
Subject: Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add
psample action
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 <amorenoz@...hat.com> 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.
Powered by blists - more mailing lists