Message-ID: <CAG=2xmMgJcir=mfQuybosg9C8j3Sx1V=Du0ObH1eT_SnBZ7nMg@mail.gmail.com>
Date: Mon, 1 Jul 2024 12:56:43 +0000
From: Adrián Moreno <amorenoz@...hat.com>
To: Michal Kubiak <michal.kubiak@...el.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 Mon, Jul 01, 2024 at 01:40:39PM GMT, Michal Kubiak wrote:
> 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
>

Hi Michal,

Thanks for sharing your thoughts.

I tried to keep the dependency between both cookie sizes loose.

I don't want a change in TC_COOKIE_MAX_SIZE to inadvertently modify
OVS_PSAMPLE_COOKIE_MAX_SIZE because OVS might not need a bigger cookie
and even if it does, backwards compatibility needs to be guaranteed:
meaning OVS userspace will have to detect the new size and use it or
fall back to a smaller cookie for older kernels. All this needs to be
known and worked on in userspace.

On the other hand, I intentionally made OVS's "psample" action as
similar as possible to act_psample, including setting the same cookie
size to begin with. The reason is that I think we should try to implement
tc-flower offloading of this action using act_sample, plus 16 seemed a
very reasonable max value.

When we decide to support offloading the "psample" action, this must
be done entirely in userspace. OVS must create a act_sample action
(instead of the OVS "psample" one) via netlink. In no circumstances the
openvswitch kmod interacts with tc directly.

Now, back to your concern. If OVS_PSAMPLE_COOKIE_MAX_SIZE grows and
TC_COOKIE_MAX_SIZE does not *and* we already support offloading this
action to tc, the only consequence is that OVS userspace has a
problem because the tc's netlink interface will reject cookies larger
than TC_COOKIE_MAX_SIZE [1].
This guarantees that the array in patch #2 is never overflown.

OVS will have to deal with the different sizes and try to squeeze the
data into TC_COOKIE_MAX_SIZE or fail to offload the action altogether.

Psample does not have a size limit so different parts of the kernel can
use psample with different internal max-sizes without any restriction.

I hope this clears your concerns.

[1] https://github.com/torvalds/linux/blob/master/net/sched/act_api.c#L1299

Thanks.
Adrián

Powered by blists - more mailing lists