On Wed, Jul 10, 2024 at 01:01:03AM GMT, Ilya Maximets wrote:
> On 7/9/24 21:07, Adrián Moreno wrote:
> > On Tue, Jul 09, 2024 at 04:15:12PM GMT, Eelco Chaudron wrote:
> >>
> >>
> >> On 9 Jul 2024, at 15:30, Adrián Moreno wrote:
> >>
> >>> On Tue, Jul 09, 2024 at 11:45:15AM GMT, Eelco Chaudron wrote:
> >>>> On 4 Jul 2024, at 9:52, Adrian Moreno wrote:
> >>>>
> >>>>> Add support for parsing and formatting the new action.
> >>>>>
> >>>>> Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
> >>>>> contains a nested OVS_ACTION_ATTR_PSAMPLE. The reason is that the
> >>>>> sampling rate form the parent "sample" is made available to the nested
> >>>>
> >>>> form -> from
> >>>>
> >>>>> "psample" by the kernel.
> >>>>
> >>>> Two small comments below, the rest looks good.
> >>>>
> >>>>> Signed-off-by: Adrian Moreno <amore...@redhat.com>
> >>>>> ---
> >>>>> include/linux/openvswitch.h | 28 +++++++++++
> >>>>> lib/dpif-netdev.c | 1 +
> >>>>> lib/dpif.c | 1 +
> >>>>> lib/odp-execute.c | 25 +++++++++-
> >>>>> lib/odp-util.c | 93 ++++++++++++++++++++++++++++++++++++
> >>>>> lib/odp-util.h | 3 ++
> >>>>> ofproto/ofproto-dpif-ipfix.c | 1 +
> >>>>> ofproto/ofproto-dpif-sflow.c | 1 +
> >>>>> python/ovs/flow/odp.py | 8 ++++
> >>>>> tests/odp.at | 16 +++++++
> >>>>> 10 files changed, 176 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> >>>>> index d9fb991ef..0023b65fb 100644
> >>>>> --- a/include/linux/openvswitch.h
> >>>>> +++ b/include/linux/openvswitch.h
> >>>>> @@ -992,6 +992,31 @@ struct check_pkt_len_arg {
> >>>>> };
> >>>>> #endif
> >>>>>
> >>>>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> >>>>> +/**
> >>>>> + * enum ovs_pample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> >>>>> + * action.
> >>>>> + *
> >>>>> + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> >>>>> + * sample.
> >>>>> + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie
> >>>>> that
> >>>>> + * contains user-defined metadata. The maximum length is
> >>>>> + * OVS_PSAMPLE_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 sample.
> >>>>> + */
> >>>>> +enum ovs_psample_attr {
> >>>>> + OVS_PSAMPLE_ATTR_GROUP = 1, /* u32 number. */
> >>>>> + OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified
> >>>>> cookie. */
> >>>>> +
> >>>>> + /* private: */
> >>>>
> >>>> None of the other definitions have this private marking, so I see no
> >>>> need to
> >>>> start adding it here.
> >>>
> >>> OK. The uAPI file has it (requested by netdev maintainers) but I guess
> >>> it's kernel-specific and this file is not a blind copy of the uAPI
> >>> anyways so I think we can remove it. I'll do on the next version.
>
> I'd keep it. There is no point in having this header different from
> the kernel one in places where it doesn't surve any purpose.
>
> >>>
> >>>>
> >>>>> + __OVS_PSAMPLE_ATTR_MAX
> >>>>> +};
> >>>>> +
> >>>>> +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> >>>>> +
> >>>>> /**
> >>>>> * enum ovs_action_attr - Action types.
> >>>>> *
> >>>>> @@ -1056,6 +1081,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_PSAMPLE: Send a sample of the packet to external
> >>>>> observers
> >>>>> + * via psample.
> >>>>> */
> >>>>>
> >>>>> enum ovs_action_attr {
> >>>>> @@ -1087,6 +1114,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 xlate_error. */
> >>>>> + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_ATTR_*. */
> >>>>>
> >>>>> #ifndef __KERNEL__
> >>>>> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
> >>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> >>>>> index c7f9e1490..f0594e5f5 100644
> >>>>> --- a/lib/dpif-netdev.c
> >>>>> +++ b/lib/dpif-netdev.c
> >>>>> @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> >>>>> *packets_,
> >>>>> case OVS_ACTION_ATTR_DROP:
> >>>>> case OVS_ACTION_ATTR_ADD_MPLS:
> >>>>> case OVS_ACTION_ATTR_DEC_TTL:
> >>>>> + case OVS_ACTION_ATTR_PSAMPLE:
> >>>>> case __OVS_ACTION_ATTR_MAX:
> >>>>> OVS_NOT_REACHED();
> >>>>> }
> >>>>> diff --git a/lib/dpif.c b/lib/dpif.c
> >>>>> index 23eb18495..71728badc 100644
> >>>>> --- a/lib/dpif.c
> >>>>> +++ b/lib/dpif.c
> >>>>> @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct
> >>>>> dp_packet_batch *packets_,
> >>>>> case OVS_ACTION_ATTR_TUNNEL_PUSH:
> >>>>> case OVS_ACTION_ATTR_TUNNEL_POP:
> >>>>> case OVS_ACTION_ATTR_USERSPACE:
> >>>>> + case OVS_ACTION_ATTR_PSAMPLE:
> >>>>> case OVS_ACTION_ATTR_RECIRC: {
> >>>>> struct dpif_execute execute;
> >>>>> struct ofpbuf execute_actions;
> >>>>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> >>>>> index 081e4d432..15577d539 100644
> >>>>> --- a/lib/odp-execute.c
> >>>>> +++ b/lib/odp-execute.c
> >>>>> @@ -818,13 +818,13 @@ requires_datapath_assistance(const struct nlattr
> >>>>> *a)
> >>>>> case OVS_ACTION_ATTR_RECIRC:
> >>>>> case OVS_ACTION_ATTR_CT:
> >>>>> case OVS_ACTION_ATTR_METER:
> >>>>> + case OVS_ACTION_ATTR_PSAMPLE:
> >>>>> return true;
> >>>>>
> >>>>> case OVS_ACTION_ATTR_SET:
> >>>>> case OVS_ACTION_ATTR_SET_MASKED:
> >>>>> case OVS_ACTION_ATTR_PUSH_VLAN:
> >>>>> case OVS_ACTION_ATTR_POP_VLAN:
> >>>>> - case OVS_ACTION_ATTR_SAMPLE:
> >>>>> case OVS_ACTION_ATTR_HASH:
> >>>>> case OVS_ACTION_ATTR_PUSH_MPLS:
> >>>>> case OVS_ACTION_ATTR_POP_MPLS:
> >>>>> @@ -841,6 +841,28 @@ requires_datapath_assistance(const struct nlattr
> >>>>> *a)
> >>>>> case OVS_ACTION_ATTR_DROP:
> >>>>> return false;
> >>>>>
> >>>>> + case OVS_ACTION_ATTR_SAMPLE: {
> >>>>> + /* Nested "psample" actions rely on the datapath executing the
> >>>>> + * parent "sample", storing the probability and making it
> >>>>> available
> >>>>> + * when the nested "psample" is run. */
> >>>>> + const struct nlattr *attr;
> >>>>> + unsigned int left;
> >>>>> +
> >>>>> + NL_NESTED_FOR_EACH (attr, left, a) {
> >>>>
> >>>> Still not hitting this code with a test case. Is this supposed to be the
> >>>> psample - local case?
> >>>
> >>> No. We had been discussing and I hadn't come up with a valid yet
> >>> reliable test when I reposted the series.
> >>>
> >>> Do you think it's OK to build a test with an inherent probability of it
> >>> failing?
> >>
> >> I assumed you would add it ;) Anyway, if 0.0015% is not sampled, just
> >> sending 100 packets would make it very unlikely that all would be dropped.
> >> Maybe just add a note to the test at the location where it would
> >> potentially fail.
> >> Haven’t thought this through, but could we do something to find out the
> >> drop reason, and retry?
> >
> > Not sure how tbh.
> > In my mind the test should:
> > - Create a flow with a sample action that xlates into:
> > sample(sample=99.99%, actions(psample(..),userspace(..))
> > - Force actions to be run in slow path.
> > - Check psample is received with the right probability which means OVS
> > has executed both the sample() and the psample() part in the kernel.
> >
> > We can send more than one packet to increase our chances.
> > We can look at ipfix stats to know if we have been very unlucky and we
> > have not fallen into the sample(), and we can retry but at some point we
> > must consider the test to have failed. So I don't know if the retry
> > helps much (compared with just doubling the number of packets sent).
> >
> > I'm going to add the test and try it a bit but I believe that (assuming
> > random() is actually random), if we configure a probability of
> > (U16_MAX-1)/U16_MAX and we repeat the experiment 100 times, that gives us
> > a really really low probability of failure.
> >
> > WDYT?
>
> I'd say 10 is enough. Sending 100 packets in tests is almost never
> a good thing to do. it just makes the tests slow. If we're so unlucky
> that none of the 10 packets hits, it means that somehting is wrong
> either in OVS or in the kernel in general.
Agree. After doing some experimentation, sending 10 packets sampled twice
and setting a probability very close to 100% I get consistent passes.
Thanks.
Adrián
>
> Alternative would be to pass probability via CMD_EXECUTE interface
> and only require datapath assistance for the psample() action itself.
>
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
