linux-kernel - Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

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: <CAG=2xmOwtdBJVk+=7KMgp1oEMuL=-OLzaWgvvV3n9Y=thnfwgA@mail.gmail.com>
Date: Tue, 2 Jul 2024 05:52:36 -0400
From: Adrián Moreno <amorenoz@...hat.com>
To: Simon Horman <horms@...nel.org>
Cc: Aaron Conole <aconole@...hat.com>, netdev@...r.kernel.org, echaudro@...hat.com, 
	i.maximets@....org, dev@...nvswitch.org, 
	Donald Hunter <donald.hunter@...il.com>, Jakub Kicinski <kuba@...nel.org>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Pravin B Shelar <pshelar@....org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

On Tue, Jul 02, 2024 at 10:37:26AM GMT, 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.
>

Ok. I'll send a new version.

Thanks.
Adrián