Re: [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Ilya Maximets <i.maximets@ovn.org>]

On 6/26/24 22:07, Adrián Moreno wrote:
> On Wed, Jun 26, 2024 at 12:51:19PM GMT, Ilya Maximets wrote:
>> On 6/25/24 22:51, Adrian Moreno wrote:
>>> Add support for a new action: emit_sample.
>>>
>>> This action accepts a u32 group id and a variable-length cookie and uses
>>> the psample multicast group to make the packet available for
>>> observability.
>>>
>>> The maximum length of the user-defined cookie is set to 16, same as
>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>>
>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>> ---
>>>  Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++
>>>  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
>>>  net/openvswitch/Kconfig                   |  1 +
>>>  net/openvswitch/actions.c                 | 45 +++++++++++++++++++++++
>>>  net/openvswitch/flow_netlink.c            | 33 ++++++++++++++++-
>>>  5 files changed, 123 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
>>> index 4fdfc6b5cae9..a7ab5593a24f 100644
>>> --- a/Documentation/netlink/specs/ovs_flow.yaml
>>> +++ b/Documentation/netlink/specs/ovs_flow.yaml
>>> @@ -727,6 +727,12 @@ attribute-sets:
>>>          name: dec-ttl
>>>          type: nest
>>>          nested-attributes: dec-ttl-attrs
>>> +      -
>>> +        name: emit-sample
>>> +        type: nest
>>> +        nested-attributes: emit-sample-attrs
>>> +        doc: |
>>> +          Sends a packet sample to psample for external observation.
>>>    -
>>>      name: tunnel-key-attrs
>>>      enum-name: ovs-tunnel-key-attr
>>> @@ -938,6 +944,17 @@ attribute-sets:
>>>        -
>>>          name: gbp
>>>          type: u32
>>> +  -
>>> +    name: emit-sample-attrs
>>> +    enum-name: ovs-emit-sample-attr
>>> +    name-prefix: ovs-emit-sample-attr-
>>> +    attributes:
>>> +      -
>>> +        name: group
>>> +        type: u32
>>> +      -
>>> +        name: cookie
>>> +        type: binary
>>>
>>>  operations:
>>>    name-prefix: ovs-flow-cmd-
>>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>>> index efc82c318fa2..8cfa1b3f6b06 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
>>>  };
>>>  #endif
>>>
>>> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
>>> +/**
>>> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
>>> + * action.
>>> + *
>>> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
>>> + * sample.
>>> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains
>>> + * user-defined metadata. The maximum length is OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
>>> + * bytes.
>>> + *
>>> + * Sends the packet to the psample multicast group with the specified group and
>>> + * cookie. It is possible to combine this action with the
>>> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted.
>>> + */
>>> +enum ovs_emit_sample_attr {
>>> +	OVS_EMIT_SAMPLE_ATTR_GROUP = 1,	/* u32 number. */
>>> +	OVS_EMIT_SAMPLE_ATTR_COOKIE,	/* Optional, user specified cookie. */
>>> +
>>> +	/* private: */
>>> +	__OVS_EMIT_SAMPLE_ATTR_MAX
>>> +};
>>> +
>>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
>>> +
>>>  /**
>>>   * enum ovs_action_attr - Action types.
>>>   *
>>> @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
>>>   * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
>>>   * argument.
>>>   * @OVS_ACTION_ATTR_DROP: Explicit drop action.
>>> + * @OVS_ACTION_ATTR_EMIT_SAMPLE: Send a sample of the packet to external
>>> + * observers via psample.
>>>   *
>>>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
>>>   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
>>> @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
>>>  	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
>>>  	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
>>>  	OVS_ACTION_ATTR_DROP,         /* u32 error code. */
>>> +	OVS_ACTION_ATTR_EMIT_SAMPLE,  /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
>>>
>>>  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
>>>  				       * from userspace. */
>>> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
>>> index 29a7081858cd..2535f3f9f462 100644
>>> --- a/net/openvswitch/Kconfig
>>> +++ b/net/openvswitch/Kconfig
>>> @@ -10,6 +10,7 @@ config OPENVSWITCH
>>>  		   (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
>>>  				     (!NF_NAT || NF_NAT) && \
>>>  				     (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT)))
>>> +	depends on PSAMPLE || !PSAMPLE
>>>  	select LIBCRC32C
>>>  	select MPLS
>>>  	select NET_MPLS_GSO
>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>> index 964225580824..1f555cbba312 100644
>>> --- a/net/openvswitch/actions.c
>>> +++ b/net/openvswitch/actions.c
>>> @@ -24,6 +24,11 @@
>>>  #include <net/checksum.h>
>>>  #include <net/dsfield.h>
>>>  #include <net/mpls.h>
>>> +
>>> +#if IS_ENABLED(CONFIG_PSAMPLE)
>>> +#include <net/psample.h>
>>> +#endif
>>> +
>>>  #include <net/sctp/checksum.h>
>>>
>>>  #include "datapath.h"
>>> @@ -1299,6 +1304,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
>>>  	return 0;
>>>  }
>>>
>>> +static void execute_emit_sample(struct datapath *dp, struct sk_buff *skb,
>>> +				const struct sw_flow_key *key,
>>
>> The 'key' is not used in the function.
>>
>>> +				const struct nlattr *attr)
>>> +{
>>> +#if IS_ENABLED(CONFIG_PSAMPLE)
>>
>> IIUC, the general coding style guideline is to compile out the whole
>> function, instead of only the parts.  i.e. something like:
>>
>> #if IS_ENABLED(CONFIG_PSAMPLE)
>> static void execute_emit_sample(...) {
>>     <body>
>> }
>> #else
>> #define execute_emit_sample(dp, skb, attr)
>> #endif
>>
>>
>> Otherwise, we'll also need to mark the arguments with __maybe_unused.
> 
> Thanks for the suggestion, will submit another version with this fix.

It seems like current coding style document prefers static inline
functions over macros in this case.  So, that might be a better
choice.  So, up to you, unless maintainers have a strong opinion one
way or another.

> 
> Adrián
> 
>>
>> The rest of the patch looks good to me.
>>
>> Best regards, Ilya Maximets.
>>
>