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