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.
>
>>>>>
>>>>> 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
