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 <[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. >
Ok. I'll send a new version. Thanks. Adrián _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
