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