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),
>>>
>>> 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
