[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZoKVtygkVYfaqjRI@localhost.localdomain>
Date: Mon, 1 Jul 2024 13:40:39 +0200
From: Michal Kubiak <michal.kubiak@...el.com>
To: Adrian Moreno <amorenoz@...hat.com>
CC: <netdev@...r.kernel.org>, <aconole@...hat.com>, <echaudro@...hat.com>,
<horms@...nel.org>, <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 Sun, Jun 30, 2024 at 09:57:26PM +0200, Adrian Moreno wrote:
> Add support for a new action: psample.
>
> 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.
>
> Acked-by: Eelco Chaudron <echaudro@...hat.com>
> Signed-off-by: Adrian Moreno <amorenoz@...hat.com>
> ---
> Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
> include/uapi/linux/openvswitch.h | 28 ++++++++++++++
> net/openvswitch/Kconfig | 1 +
> net/openvswitch/actions.c | 47 +++++++++++++++++++++++
> net/openvswitch/flow_netlink.c | 32 ++++++++++++++-
> 5 files changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..46f5d1cd8a5f 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: psample
> + type: nest
> + nested-attributes: psample-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: psample-attrs
> + enum-name: ovs-psample-attr
> + name-prefix: ovs-psample-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..3dd653748725 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_PSAMPLE_COOKIE_MAX_SIZE 16
In your patch #2 you use "TC_COOKIE_MAX_SIZE" as an array size for your
cookie. I know that now OVS_PSAMPLE_COOKIE_MAX_SIZE == TC_COOKIE_MAX_SIZE,
so this size will be validated correctly.
But how likely is that those 2 constants will have different values in the
future?
Would it be reasonable to create more strict dependency between those
macros, e.g.:
#define OVS_PSAMPLE_COOKIE_MAX_SIZE TC_COOKIE_MAX_SIZE
or, at least, add a comment that the size shouldn't be bigger than
TC_COOKIE_MAX_SIZE?
I'm just considering the risk of exceeding the array from the patch #2 when
somebody increases OVS_PSAMPLE_COOKIE_MAX_SIZE in the future.
Thanks,
Michal
Powered by blists - more mailing lists