On 20/03/2025 14:51, Ilya Maximets wrote:
> On 3/20/25 09:32, Roi Dayan wrote:
>>
>>
>> On 13/03/2025 11:45, Eelco Chaudron wrote:
>>>
>>>
>>> On 27 Jan 2025, at 17:19, Roi Dayan wrote:
>>>
>>>> On 22/01/2025 15:02, Roi Dayan wrote:
>>>>>
>>>>>
>>>>> On 22/01/2025 14:30, Ilya Maximets wrote:
>>>>>> On 1/22/25 13:02, Roi Dayan via dev wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 22/01/2025 11:34, Eelco Chaudron wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 22 Jan 2025, at 8:37, Roi Dayan wrote:
>>>>>>>>
>>>>>>>>> On 21/01/2025 15:02, Ilya Maximets wrote:
>>>>>>>>>> On 1/20/25 16:13, Eelco Chaudron wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 20 Jan 2025, at 14:50, Roi Dayan wrote:
>>>>>>>>>>>
>>>>>>>>>>>> On 20/01/2025 14:57, Eelco Chaudron wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 15 Jan 2025, at 10:18, Roi Dayan via dev wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Use this instead of directly using dpif_is_netdev() from
>>>>>>>>>>>>>> dpif-netdev.
>>>>>>>>>>>>>> Return true in dpif-netdev.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Not sure if we need an API for this, or maybe I should say, what
>>>>>>>>>>>>> is the definition
>>>>>>>>>>>>> of userspace? If this means anything but the kernel, we could
>>>>>>>>>>>>> probably use !dpif_is_netlink().
>>>>>>>>>>>>>
>>>>>>>>>>>>> But I guess, the better question would be, what would we use this
>>>>>>>>>>>>> call for? If it’s similar
>>>>>>>>>>>>> to the existing use cases, we are probably better off by adding a
>>>>>>>>>>>>> features API to dpif.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thoughts?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Eelco
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> The meaning of dpif is a userspace means it needs to handle
>>>>>>>>>>>> tunneling and other stuff that
>>>>>>>>>>>> kernel dpif doesn't need as the kernel does it for it.
>>>>>>>>>>>> So while !dpif_is_netlink() is even more correct and better than
>>>>>>>>>>>> current dpif_is_netdev()
>>>>>>>>>>>> there was also a point here that dpif.c is not familiar with a
>>>>>>>>>>>> specific dpif and not accessing
>>>>>>>>>>>> a specific dpif function.
>>>>>>>>>>>
>>>>>>>>>>> So, this means a lot of potential stuff, so to me a general
>>>>>>>>>>> dpif_is_userspace() does not make sense.
>>>>>>>>>>>
>>>>>>>>>>>> I think introducing a features API to dpif could be an overkill at
>>>>>>>>>>>> this point.
>>>>>>>>>>>> So if future dpifs might need something specific other than
>>>>>>>>>>>> handled by kernel or not I think
>>>>>>>>>>>> this can be done at a later time.
>>>>>>>>>>>
>>>>>>>>>>> It might look like an overkill right now, but the current use cases
>>>>>>>>>>> might warrant one already.
>>>>>>>>>>> Or are you suggesting to use/introduce !dpif_is_netlink() instead
>>>>>>>>>>> for now?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Ilya and other maintainers have any preferences or thoughts on this.
>>>>>>>>>>
>>>>>>>>>> In general, using either dpif_is_netlink or dpif_is_netdev outside
>>>>>>>>>> of dpif-netlink
>>>>>>>>>> and dpif-netdev respectively is icky. And I agree that even today
>>>>>>>>>> the number of
>>>>>>>>>> uses of these functions outside their modules justifies doing the
>>>>>>>>>> feature discovery
>>>>>>>>>> properly. Majority of the cases here in dpif.c are caused by the
>>>>>>>>>> fact that
>>>>>>>>>> dpif-netdev doesn't validate actions, so it's not possible to probe
>>>>>>>>>> them. It may be
>>>>>>>>>> better to introduce some form of action validation in case of
>>>>>>>>>> probing instead, so
>>>>>>>>>> the features can be discovered normally.
>>>>>>>>>>
>>>>>>>>>> Best regards, Ilya Maximets.
>>>>>>>>>
>>>>>>>>> I think this is out-of-scope for this patch.
>>>>>>>>> In this small commit I just wanted to do the separation from dpif.c
>>>>>>>>> knowing
>>>>>>>>> about existing dpifs and accessing them directly and not introducing
>>>>>>>>> new layers and mechanisem.
>>>>>>
>>>>>> Feature probing is not a new mechanism. We do that for many other
>>>>>> actions
>>>>>> and it is a primary mechanism for a datapath feature discovery.
>>>>>>
>>>>>>>>> Can we continue with this patch currently? a rename to the callback
>>>>>>>>> is also feasible though
>>>>>>>>> I think its saying exactly what it is.
>>>>>>>>
>>>>>>>> This patch introduces a new API, but its purpose is unclear to me.
>>>>>>>> While it’s a small change,
>>>>>>>> I don't see a concrete use case for it. The term 'userspace' seems
>>>>>>>> vague when used outside the
>>>>>>>> context of the actual dpif. Could you provide a clear example of how
>>>>>>>> this API will be used?
>>>>>>>> This might help us explore better alternative approaches if needed.
>>>>>>>> For the current use cases
>>>>>>>> see Ilya’s response.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>>
>>>>>>>> Eelco
>>>>>>>>
>>>>>>>
>>>>>>> A concrete example is tunnel handling. In "netdev" datapath, OVS
>>>>>>> userspace application handles
>>>>>>> tunnel push and pop, as opposed to "netlink" datapath in which the
>>>>>>> kernel handles it.
>>>>>>> A more suitable name for it in the first place was "is_userspace".
>>>>>>> We want to add another userspace datapath (dpif-doca), that will handle
>>>>>>> tunnels, therefore this
>>>>>>> change.
>>>>>>
>>>>>> This is exactly the use case for a feature probing. How we do that for
>>>>>> other
>>>>>> matches and actions is constructing a dummy flow rule, calling flow_put
>>>>>> with a
>>>>>> probe flag set and letting datapath validate it. This works for various
>>>>>> versions
>>>>>> of linux kernel datapath that can be running and that works for windows
>>>>>> datapath.
>>>>>> This works fine for userspace datapath as well, except only for matches.
>>>>>> For
>>>>>> actions it doesn't work, because the userspace datapath doesn't validate
>>>>>> actions.
>>>>>> If we plan to have multiple implementations for userspace-style
>>>>>> datpaths, then we
>>>>>> should probe them properly as we do for multiple implementations of
>>>>>> dpif-netlink.
>>>>>
>>>>> Are you talking about the TC probing in netdev-offload-tc ? those probing
>>>>> is not being done from the dpif.c.
>>>>> TC offloading checks the probes there and either offload or not.
>>>>> Maybe all those wrappers checking dpif_is_netdev in dpif.c should not
>>>>> exists then and maybe all actions should just be passed into the dpif and
>>>>> the dpif itself should decide what to do with the actions. dpif-netlink
>>>>> should decide if to skip the actions or not.
>>>>> All this seems out of scope of keeping the same behavior as it but just
>>>>> opening it a bit without touching the specific logic that exists now.
>>>>> Moving to the proposal using !dpif_is_netlink also solves the issue for
>>>>> userspace dpif implementations.
>>>>> if not, I don't understand what kind of probing is expected here. can you
>>>>> share an example?
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> Just updating here that point is not to change the API or logic of the
>>>> callers using the dpif_is_netdev.
>>>>
>>>> dpif_supports_tnl_push_pop
>>>> dpif_may_support_explicit_drop_action
>>>> dpif_supports_lb_output_action
>>>> dpif_may_support_psample
>>>>
>>>> My intention that if I test with a new userspace dpif then not getting
>>>> blocked by dpif that those features are not supported just because its not
>>>> specific dpif-netdev.
>>>> Please provide an example how you suggest you proceed here.
>>>
>>> I think what Ilya proposed is to add proper action verification for
>>> dpif-netdev, so we can get rid of the psample instance, and then you change
>>> the remaining cases to actual probes. Ilya, correct me if I’m wrong.
>>>
>>> For the netdev part, see a quick patch below;
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index 87d69c46d..e6b7131d4 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -4307,6 +4307,60 @@ exit:
>>> return error;
>>> }
>>>
>>> +static bool
>>> +dpif_netdev_actions_supported(const struct nlattr *actions, size_t
>>> actions_len)
>>> +{
>>> + const struct nlattr *nla;
>>> + size_t left;
>>> +
>>> + if (!actions_len) {
>>> + return true;
>>> + }
>>> +
>>> + NL_ATTR_FOR_EACH(nla, left, actions, actions_len) {
>>> + enum ovs_action_attr type = nl_attr_type(nla);
>>> +
>>> + switch (type) {
>>> + /* Supported actions go here in order of definition. */
>>> + case OVS_ACTION_ATTR_OUTPUT:
>>> + case OVS_ACTION_ATTR_USERSPACE:
>>> + case OVS_ACTION_ATTR_SET:
>>> + case OVS_ACTION_ATTR_PUSH_VLAN:
>>> + case OVS_ACTION_ATTR_POP_VLAN:
>>> + case OVS_ACTION_ATTR_SAMPLE:
>>> + case OVS_ACTION_ATTR_RECIRC:
>>> + case OVS_ACTION_ATTR_HASH:
>>> + case OVS_ACTION_ATTR_PUSH_MPLS:
>>> + case OVS_ACTION_ATTR_POP_MPLS:
>>> + case OVS_ACTION_ATTR_SET_MASKED:
>>> + case OVS_ACTION_ATTR_CT:
>>> + case OVS_ACTION_ATTR_TRUNC:
>>> + case OVS_ACTION_ATTR_PUSH_ETH:
>>> + case OVS_ACTION_ATTR_POP_ETH:
>>> + case OVS_ACTION_ATTR_CT_CLEAR:
>>> + case OVS_ACTION_ATTR_PUSH_NSH:
>>> + case OVS_ACTION_ATTR_POP_NSH:
>>> + case OVS_ACTION_ATTR_METER:
>>> + case OVS_ACTION_ATTR_CLONE:
>>> + case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>>> + case OVS_ACTION_ATTR_ADD_MPLS:
>>> + case OVS_ACTION_ATTR_DEC_TTL:
>>> + case OVS_ACTION_ATTR_DROP:
>>> + case OVS_ACTION_ATTR_TUNNEL_PUSH:
>>> + case OVS_ACTION_ATTR_TUNNEL_POP:
>>> + case OVS_ACTION_ATTR_LB_OUTPUT:
>>> + return true;
>>> +
>>> + /* Unsupported actions go here in order of definition. */
>>> + case OVS_ACTION_ATTR_UNSPEC:
>>> + case OVS_ACTION_ATTR_PSAMPLE:
>>> + case __OVS_ACTION_ATTR_MAX:
>>> + break;
>>> + }
>>> + }
>>> + return false;
>>> +}
>>> +
>>> static int
>>> dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put)
>>> {
>>> @@ -4367,6 +4421,13 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct
>>> dpif_flow_put *put)
>>> * address mask. Installation of the flow will use the match variable.
>>> */
>>> netdev_flow_key_init(&key, &match.flow);
>>>
>>> + /* For probe operations, verify if the individual actions are
>>> supported.
>>> + * Otherwise, assume the higher layer, ofproto, manages action
>>> support. */
>>> + if (probe && !dpif_netdev_actions_supported(put->actions,
>>> + put->actions_len)) {
>>> + return EINVAL;
>>> + }
>>> +
>>> if (put->pmd_id == PMD_ID_NULL) {
>>> if (cmap_count(&dp->poll_threads) == 0) {
>>> return EINVAL;
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index a064f717f..c22a47c3c 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -1953,13 +1953,6 @@ dpif_supports_lb_output_action(const struct dpif
>>> *dpif)
>>> return dpif_is_netdev(dpif);
>>> }
>>>
>>> -bool
>>> -dpif_may_support_psample(const struct dpif *dpif)
>>> -{
>>> - /* Userspace datapath does not support this action. */
>>> - return !dpif_is_netdev(dpif);
>>> -}
>>> -
>>> /* Meters */
>>> void
>>> dpif_meter_get_features(const struct dpif *dpif,
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index bf43d5d4b..7637d93de 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -1646,8 +1646,8 @@ check_psample(struct dpif_backer *backer)
>>>
>>> odp_put_psample_action(&actions, 10, cookie, sizeof cookie);
>>>
>>> - supported = dpif_may_support_psample(backer->dpif) &&
>>> - dpif_probe_feature(backer->dpif, "psample", &key, &actions, NULL);
>>> + supported = dpif_probe_feature(backer->dpif, "psample", &key, &actions,
>>> + NULL);
>>>
>>> ofpbuf_uninit(&actions);
>>> VLOG_INFO("%s: Datapath %s psample action", dpif_name(backer->dpif),
>>>
>>>
>>>
>>
>> thanks Eelco, I'll take a look at this example and see what I come with as a
>> different patch.
>
> Sorry, I lost track of this duscussion at some point.
>
> Eelco, yes, your reference code above matches my previous suggestion.
> A few small comments though:
> 1. The check should be recursive, since we have nested actions.
> 2. dpif-netdev should only check actions it implements on it's own and
> call a verification helper for all other actions implemented in the
> odp-execute.c. That helper may return true for all the actions it
> supports directly and false for all the actions that require datapath
> assistance.
>
> Other than that and the indentation of the cases, it looks good. :)
>
> Best regards, Ilya Maximets.
>
Thanks for the suggestions and comments.
I submitted a different series according the suggestions
instead of this patch:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=450922
>>
>>>>>>
>>>>>> This is_userspace call is actually a request "if this implementation
>>>>>> supports
>>>>>> the following actions: tunnel_push_pop, explicit drop, lb_output and
>>>>>> psample?".
>>>>>> Answer "true" corresponds to "yes, yes, yes, no", and "false" means "no,
>>>>>> maybe,
>>>>>> no, maybe", and then dpif.c proceeds to probe for "maybe" actions.
>>>>>>
>>>>>> So, this call is only a shortcut with non-obvious overloaded meaning
>>>>>> that will
>>>>>> change over time as we add new actions. Instead, we can remove it and
>>>>>> just do
>>>>>> probing as dpif layer already does for most of other matches and actions.
>>>>>> It should also be easier for you to maintain an alternative userspace
>>>>>> datapath
>>>>>> implementation this way, as you'll not need to worry about what this call
>>>>>> actually means, as dpif layer would just probe what it needs directly
>>>>>> instead
>>>>>> of assuming things based on a single boolean value.
>>>>>>
>>>>>> Best regards, Ilya Maximets.
>>>>>
>>>
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
