On 04/04/2025 13:31, Ilya Maximets wrote:
> On 4/4/25 12:17 PM, Eelco Chaudron wrote:
>>
>>
>> On 2 Apr 2025, at 9:32, Roi Dayan wrote:
>>
>>> Calling dpif_probe_feature() should return if supported or not
>>> without relation to the dpif name.
>>> Remove the redundant recheck of drop support.
>>> TC is supporting drop action from the following commit.
>>> [1] 002682727e6e ("netdev-offload-tc: Add drop action support.")
>>
>> Hi Roi,
>>
>> This is for drop support, not explicit drop support with reasoning,
>> so just removing this code will not work.
>
> Hmm, the cited commit is also not correct. It only adds support
> for OVS action to TC translation, but doesn't cover the TC to OVS
> action convertion back. So, after a flow dump we'll have recirc(0)
> instead of drop and OVS will be in a constant revalidation cycle,
> removing and re-inserting this rule.
>
> This doesn't happen for userspace datapath (for which this commit
> was added) because userspace datapath doesn't actually dump offloaded
> flows back. But also, I'm not sure if it's even possible to run
> userspace datapath + tc offload without extra changes.
>
> Best regards, Ilya Maximets.
>
>> Also, this patch is failing quite some regressions.
>>
>> //Eelco
>>
>> PS: It might be a good idea to enable testing on your local GitHub repo
>> and push your patches there first. That way, you can make sure your
>> changes are tested before sending them upstream!
>>
>>
Thanks for the comments and suggestions. I think I only tested "make check".
I'll check the other tests and also pushed to github to get a better view and
change what's needed locally as well.
I'll go over your suggestions.
>>> Signed-off-by: Roi Dayan <r...@nvidia.com>
>>> Reviewed-by: Eli Britstein <el...@nvidia.com>
>>> ---
>>> lib/dpif.c | 7 -------
>>> lib/dpif.h | 1 -
>>> ofproto/ofproto-dpif.c | 31 ++-----------------------------
>>> 3 files changed, 2 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index c22a47c3c15e..58ef90a5ded0 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -1936,13 +1936,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
>>> return dpif_is_netdev(dpif);
>>> }
>>>
>>> -bool
>>> -dpif_may_support_explicit_drop_action(const struct dpif *dpif)
>>> -{
>>> - /* TC does not support offloading this action. */
>>> - return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
>>> -}
>>> -
>>> bool
>>> dpif_supports_lb_output_action(const struct dpif *dpif)
>>> {
>>> diff --git a/lib/dpif.h b/lib/dpif.h
>>> index a764e8a592bd..8dc5da45b3ef 100644
>>> --- a/lib/dpif.h
>>> +++ b/lib/dpif.h
>>> @@ -940,7 +940,6 @@ int dpif_get_pmds_for_port(const struct dpif * dpif,
>>> odp_port_t port_no,
>>>
>>> char *dpif_get_dp_version(const struct dpif *);
>>> bool dpif_supports_tnl_push_pop(const struct dpif *);
>>> -bool dpif_may_support_explicit_drop_action(const struct dpif *);
>>> bool dpif_synced_dp_layers(struct dpif *);
>>>
>>> /* Log functions. */
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index 7637d93de73e..b8b32b5778e7 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -223,7 +223,6 @@ static void ct_zone_config_init(struct dpif_backer
>>> *backer);
>>> static void ct_zone_config_uninit(struct dpif_backer *backer);
>>> static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
>>> static void ct_zone_limits_commit(struct dpif_backer *backer);
>>> -static bool recheck_support_explicit_drop_action(struct dpif_backer
>>> *backer);
>>>
>>> static inline struct ofproto_dpif *
>>> ofproto_dpif_cast(const struct ofproto *ofproto)
>>> @@ -394,10 +393,6 @@ type_run(const char *type)
>>> udpif_set_threads(backer->udpif, n_handlers, n_revalidators);
>>> }
>>>
>>> - if (recheck_support_explicit_drop_action(backer)) {
>>> - backer->need_revalidate = REV_RECONFIGURE;
>>> - }
>>> -
>>> if (backer->need_revalidate) {
>>> struct ofproto_dpif *ofproto;
>>> struct simap_node *node;
>>> @@ -1420,8 +1415,8 @@ check_drop_action(struct dpif_backer *backer)
>>> ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf);
>>> nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK);
>>>
>>> - supported = dpif_may_support_explicit_drop_action(backer->dpif) &&
>>> - dpif_probe_feature(backer->dpif, "drop", &key, &actions,
>>> NULL);
>>> + supported = dpif_probe_feature(backer->dpif, "drop", &key, &actions,
>>> + NULL);
>>>
>>> VLOG_INFO("%s: Datapath %s explicit drop action",
>>> dpif_name(backer->dpif),
>>> @@ -1757,28 +1752,6 @@ check_support(struct dpif_backer *backer)
>>> backer->rt_support.odp.nd_ext = check_nd_extensions(backer);
>>> }
>>>
>>> -/* TC does not support offloading the explicit drop action. As such we
>>> need to
>>> - * re-probe the datapath if hw-offload has been modified.
>>> - * Note: We don't support true --> false transition as that requires a
>>> restart.
>>> - * See netdev_set_flow_api_enabled(). */
>>> -static bool
>>> -recheck_support_explicit_drop_action(struct dpif_backer *backer)
>>> -{
>>> - bool explicit_drop_action;
>>> -
>>> - atomic_read_relaxed(&backer->rt_support.explicit_drop_action,
>>> - &explicit_drop_action);
>>> -
>>> - if (explicit_drop_action
>>> - && !dpif_may_support_explicit_drop_action(backer->dpif)) {
>>> - ovs_assert(!check_drop_action(backer));
>>> - atomic_store_relaxed(&backer->rt_support.explicit_drop_action,
>>> false);
>>> - return true;
>>> - }
>>> -
>>> - return false;
>>> -}
>>> -
>>> static int
>>> construct(struct ofproto *ofproto_)
>>> {
>>> --
>>> 2.21.0
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
