On 4/3/24 00:28, Eric Garver wrote: > On Fri, Mar 29, 2024 at 12:26:11AM +0100, Ilya Maximets wrote: >> On 3/22/24 14:54, Eric Garver wrote: > [..] >>> @@ -1649,8 +1693,8 @@ check_support(struct dpif_backer *backer) >>> backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer); >>> backer->rt_support.check_pkt_len = check_check_pkt_len(backer); >>> backer->rt_support.ct_timeout = check_ct_timeout_policy(backer); >>> - backer->rt_support.explicit_drop_action = >>> - dpif_supports_explicit_drop_action(backer->dpif); >>> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, >>> + check_drop_action(backer)); >>> backer->rt_support.lb_output_action = >>> dpif_supports_lb_output_action(backer->dpif); >>> backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer); >>> @@ -1667,6 +1711,28 @@ 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) >>> + && !check_drop_action(backer)) { >> >> Shouldn't last two conditions be || instad of && ? > > Assuming you mean: > > if (explicit_drop_action && > (!dpif_may_support_explicit_drop_action(backer->dpif) || > !check_drop_action(backer))) { > > Then I don't think so. This function is periodically called from > type_run(). If we use || here, then the usual case is that > dpif_may_support_explicit_drop_action() will return true, i.e. > hw-offload=false. That would force check_drop_action() to be called. > > Basically we want to avoid calling check_drop_action() by guarding it > with the much cheaper dpif_may_support_explicit_drop_action().
Hmm, OK. But why do we call check_drop_action() here at all then? Just for the log message? Maybe it's better then to do something like: if (explicit_drop_action && && !dpif_may_support_explicit_drop_action(backer->dpif)) { ovs_assert(!check_drop_action(backer)); atomic_store_relaxed( ..., false); return true; } What do you think? Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev