On Wed, Apr 03, 2024 at 12:42:25AM +0200, Ilya Maximets wrote:
> 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?
Yeah, I guess just for the log.
> 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?
That will work. Alternatively we could add a VLOG_INFO() here. I like
the assert though as it keeps everything neatly in check_drop_action().
> Best regards, Ilya Maximets.
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
