Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action
From: Simon Horman
Date: Tue Jul 02 2024 - 05:37:41 EST
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@xxxxxxxxxx> 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.
...