On 5/10/24 13:03, Adrian Moreno wrote:
>
>
> On 5/10/24 12:06 PM, Eelco Chaudron wrote:
>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>>
>>> Only kernel datapath supports psample so check that the datapath is not
>>> userspace and that it accepts the new attributes.
>>>
>>> Signed-off-by: Adrian Moreno <amore...@redhat.com>
>>> ---
>>> ofproto/ofproto-dpif.c | 59 ++++++++++++++++++++++++++++++++++++++++++
>>> ofproto/ofproto-dpif.h | 6 ++++-
>>> 2 files changed, 64 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index 32d037be6..3cee2795a 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -25,6 +25,7 @@
>>> #include "coverage.h"
>>> #include "cfm.h"
>>> #include "ct-dpif.h"
>>> +#include "dpif-netdev.h"
>>> #include "fail-open.h"
>>> #include "guarded-list.h"
>>> #include "hmapx.h"
>>> @@ -873,6 +874,12 @@ ovs_lb_output_action_supported(struct ofproto_dpif
>>> *ofproto)
>>> return ofproto->backer->rt_support.lb_output_action;
>>> }
>>>
>>> +bool
>>> +ovs_psample_supported(struct ofproto_dpif *ofproto)
>>
>> As mentioned in the previous patch, I do not like the psample terminology.
>> It refers to an implementation-specific name, we should come up with an
>> agnostic name, like ’system/local’, but there are probably better names.
>>
>>> +{
>>> + return ofproto->backer->rt_support.psample;
>>> +}
>>> +
>>> /* Tests whether 'backer''s datapath supports recirculation. Only newer
>>> * datapaths support OVS_KEY_ATTR_RECIRC_ID in keys. We need to disable
>>> some
>>> * features on older datapaths that don't support this feature.
>>> @@ -1440,6 +1447,14 @@ dpif_supports_ct_zero_snat(struct dpif_backer
>>> *backer)
>>> return supported;
>>> }
>>>
>>> +static bool check_psample(struct dpif_backer *backer);
>>> +
>>> +static bool
>>> +dpif_supports_psample(struct dpif_backer *backer)
>>> +{
>>> + return !dpif_is_netdev(backer->dpif) && check_psample(backer);
>>> +}
>>
>> This looks odd, we should not do any datapath specific checks here. We
>> should just call check_psample() on the netdev datapath, and it should fail.
>> If we ever add support for a similar feature we need to remove this code
>> anyway. Guess it also omits the log message for userspace.
>>
>
> I agree. Unfortunately, I didn't see any action verification code in the
> netdev
> datapath that (as the kernel does) would make the action installation fail.
> I'll
> double check but I think it's just not capable of detecting unsupported
> attributes on flow installation.
It's expected that userspace datapath is a reference implementation that
supports any actions, because it executes most of the actions via odp-execute
module that supposed to know all the actions.
This one however doesn't make sense for userspace datapath, so we have to
check beforehand. Adding extra parsing of actions may have significant
performance impact for upcalls.
However, ofproto-dpif module should not know any datapath details and must
not include dpif-netdev.h, the logic should be moved to dpif.c. For example,
see what we did for the explicit drop action in
dpif_may_support_explicit_drop_action().
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
