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.
>>>>
>>>> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
