Re: [ovs-dev] [PATCH v13 4/6] dpif: Probe support for OVS_ACTION_ATTR_DROP.
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(>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(>rt_support.explicit_drop_action,
> >>> +_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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v13 4/6] dpif: Probe support for OVS_ACTION_ATTR_DROP.
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(>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(>rt_support.explicit_drop_action,
>>> +_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
Re: [ovs-dev] [PATCH v13 4/6] dpif: Probe support for OVS_ACTION_ATTR_DROP.
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(>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(>rt_support.explicit_drop_action,
> > +_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().
> > +atomic_store_relaxed(>rt_support.explicit_drop_action,
> > false);
> > +return true;
> > +}
> > +
> > +return false;
> > +}
> > +
[..]
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v13 4/6] dpif: Probe support for OVS_ACTION_ATTR_DROP.
On 4/2/24 16:22, 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:
>>> Kernel support has been added for this action. As such, we need to probe
>>> the datapath for support.
>>>
>>> Acked-by: Eelco Chaudron
>>> Signed-off-by: Eric Garver
>>> ---
>>> include/linux/openvswitch.h | 2 +-
>>> lib/dpif.c | 6 ++-
>>> lib/dpif.h | 2 +-
>>> ofproto/ofproto-dpif.c | 78 ++---
>>> ofproto/ofproto-dpif.h | 4 +-
>>> 5 files changed, 82 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>> index a265e05ad253..fce6456f47c8 100644
>>> --- a/include/linux/openvswitch.h
>>> +++ b/include/linux/openvswitch.h
>>> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
>>> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
>>> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>>>
>>> #ifndef __KERNEL__
>>> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
>>> OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
>>> - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>>> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
>>> #endif
>>> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index 0f480bec48d0..23eb18495a66 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -28,6 +28,7 @@
>>> #include "dpctl.h"
>>> #include "dpif-netdev.h"
>>> #include "flow.h"
>>> +#include "netdev-offload.h"
>>> #include "netdev-provider.h"
>>> #include "netdev.h"
>>> #include "netlink.h"
>>> @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
>>> }
>>>
>>> bool
>>> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
>>> +dpif_may_support_explicit_drop_action(const struct dpif *dpif)
>>> {
>>> -return dpif_is_netdev(dpif);
>>> +/* TC does not support offloading this action. */
>>> +return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
>>> }
>>>
>>> bool
>>> diff --git a/lib/dpif.h b/lib/dpif.h
>>> index 0f2dc2ef3c55..a764e8a592bd 100644
>>> --- a/lib/dpif.h
>>> +++ b/lib/dpif.h
>>> @@ -940,7 +940,7 @@ 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_supports_explicit_drop_action(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 c4e2e867ecdc..b854ba3636ed 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -221,6 +221,7 @@ 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)
>>> @@ -391,6 +392,10 @@ 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;
>>> @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer
>>> **backerp)
>>>
>>> shash_add(_dpif_backers, type, backer);
>>>
>>> +atomic_init(>rt_support.explicit_drop_action, false);
>>
>> This initialization seems unnecessary since the field will be initialized
>> in the check_support() right after. And it also a bit out of place, since
>> we are not initializing anything else.
>
> Hrm.. It's _set_ in check_support(), my initial understanding of
> ovs-atomics.h was that they also needed to be _initialized_. However,
> all but clang has the same assignment macro for atomic_init(). This
> macro is used throughout the code.
>
> What do you think?
As long as the value is not used before setting and compiler doesn't
complain it should be OK to not initialize it.
If the compiler complains or will start complaining at some point (there
should be no reason for that), I'd prefer we replace xmalloc with xzalloc
instead of explicitly initializing this particular field.
Best regards, Ilya Maximets.
Re: [ovs-dev] [PATCH v13 4/6] dpif: Probe support for OVS_ACTION_ATTR_DROP.
On Fri, Mar 29, 2024 at 12:26:11AM +0100, Ilya Maximets wrote:
> On 3/22/24 14:54, Eric Garver wrote:
> > Kernel support has been added for this action. As such, we need to probe
> > the datapath for support.
> >
> > Acked-by: Eelco Chaudron
> > Signed-off-by: Eric Garver
> > ---
> > include/linux/openvswitch.h | 2 +-
> > lib/dpif.c | 6 ++-
> > lib/dpif.h | 2 +-
> > ofproto/ofproto-dpif.c | 78 ++---
> > ofproto/ofproto-dpif.h | 4 +-
> > 5 files changed, 82 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> > index a265e05ad253..fce6456f47c8 100644
> > --- a/include/linux/openvswitch.h
> > +++ b/include/linux/openvswitch.h
> > @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
> > OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> > + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> >
> > #ifndef __KERNEL__
> > OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
> > OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
> > - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> > OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
> > #endif
> > __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> > diff --git a/lib/dpif.c b/lib/dpif.c
> > index 0f480bec48d0..23eb18495a66 100644
> > --- a/lib/dpif.c
> > +++ b/lib/dpif.c
> > @@ -28,6 +28,7 @@
> > #include "dpctl.h"
> > #include "dpif-netdev.h"
> > #include "flow.h"
> > +#include "netdev-offload.h"
> > #include "netdev-provider.h"
> > #include "netdev.h"
> > #include "netlink.h"
> > @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
> > }
> >
> > bool
> > -dpif_supports_explicit_drop_action(const struct dpif *dpif)
> > +dpif_may_support_explicit_drop_action(const struct dpif *dpif)
> > {
> > -return dpif_is_netdev(dpif);
> > +/* TC does not support offloading this action. */
> > +return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
> > }
> >
> > bool
> > diff --git a/lib/dpif.h b/lib/dpif.h
> > index 0f2dc2ef3c55..a764e8a592bd 100644
> > --- a/lib/dpif.h
> > +++ b/lib/dpif.h
> > @@ -940,7 +940,7 @@ 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_supports_explicit_drop_action(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 c4e2e867ecdc..b854ba3636ed 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -221,6 +221,7 @@ 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)
> > @@ -391,6 +392,10 @@ 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;
> > @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer
> > **backerp)
> >
> > shash_add(_dpif_backers, type, backer);
> >
> > +atomic_init(>rt_support.explicit_drop_action, false);
>
> This initialization seems unnecessary since the field will be initialized
> in the check_support() right after. And it also a bit out of place, since
> we are not initializing anything else.
Hrm.. It's _set_ in check_support(), my initial understanding of
ovs-atomics.h was that they also needed to be _initialized_. However,
all but clang has the same assignment macro for atomic_init(). This
macro is used throughout the code.
What do you think?
> > check_support(backer);
> > atomic_count_init(>tnl_count, 0);
> >
> > @@ -855,7 +861,12 @@ ovs_native_tunneling_is_on(struct ofproto_dpif
> > *ofproto)
> > bool
> > ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto)
> > {
> > -return ofproto->backer->rt_support.explicit_drop_action;
> > +bool value;
> > +
> > +atomic_read_relaxed(>backer->rt_support.explicit_drop_action,
> > +
Re: [ovs-dev] [PATCH v13 4/6] dpif: Probe support for OVS_ACTION_ATTR_DROP.
On 3/29/24 00:26, Ilya Maximets wrote:
> On 3/22/24 14:54, Eric Garver wrote:
>> Kernel support has been added for this action. As such, we need to probe
>> the datapath for support.
>>
>> Acked-by: Eelco Chaudron
>> Signed-off-by: Eric Garver
>> ---
>> include/linux/openvswitch.h | 2 +-
>> lib/dpif.c | 6 ++-
>> lib/dpif.h | 2 +-
>> ofproto/ofproto-dpif.c | 78 ++---
>> ofproto/ofproto-dpif.h | 4 +-
>> 5 files changed, 82 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index a265e05ad253..fce6456f47c8 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
>> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
>> +OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>>
>> #ifndef __KERNEL__
>> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
>> OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
>> -OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
>> #endif
>> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 0f480bec48d0..23eb18495a66 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -28,6 +28,7 @@
>> #include "dpctl.h"
>> #include "dpif-netdev.h"
>> #include "flow.h"
>> +#include "netdev-offload.h"
>> #include "netdev-provider.h"
>> #include "netdev.h"
>> #include "netlink.h"
>> @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
>> }
>>
>> bool
>> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
>> +dpif_may_support_explicit_drop_action(const struct dpif *dpif)
>> {
>> -return dpif_is_netdev(dpif);
>> +/* TC does not support offloading this action. */
>> +return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
>> }
>>
>> bool
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index 0f2dc2ef3c55..a764e8a592bd 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -940,7 +940,7 @@ 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_supports_explicit_drop_action(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 c4e2e867ecdc..b854ba3636ed 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -221,6 +221,7 @@ 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)
>> @@ -391,6 +392,10 @@ 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;
>> @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer
>> **backerp)
>>
>> shash_add(_dpif_backers, type, backer);
>>
>> +atomic_init(>rt_support.explicit_drop_action, false);
>
> This initialization seems unnecessary since the field will be initialized
> in the check_support() right after. And it also a bit out of place, since
> we are not initializing anything else.
>
>> check_support(backer);
>> atomic_count_init(>tnl_count, 0);
>>
>> @@ -855,7 +861,12 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto)
>> bool
>> ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto)
>> {
>> -return ofproto->backer->rt_support.explicit_drop_action;
>> +bool value;
>> +
>> +atomic_read_relaxed(>backer->rt_support.explicit_drop_action,
>> +);
>> +
>
> May remove te empty lne here.
>
>> +return value;
>> }
>>
>> bool
>> @@ -1379,6 +1390,39 @@ check_ct_timeout_policy(struct dpif_backer *backer)
>> return !error;
>> }
>>
>> +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP
>> action. */
>> +static bool
>> +check_drop_action(struct dpif_backer *backer)
>> +{
>> +struct
Re: [ovs-dev] [PATCH v13 4/6] dpif: Probe support for OVS_ACTION_ATTR_DROP.
On 3/22/24 14:54, Eric Garver wrote:
> Kernel support has been added for this action. As such, we need to probe
> the datapath for support.
>
> Acked-by: Eelco Chaudron
> Signed-off-by: Eric Garver
> ---
> include/linux/openvswitch.h | 2 +-
> lib/dpif.c | 6 ++-
> lib/dpif.h | 2 +-
> ofproto/ofproto-dpif.c | 78 ++---
> ofproto/ofproto-dpif.h | 4 +-
> 5 files changed, 82 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index a265e05ad253..fce6456f47c8 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>
> #ifndef __KERNEL__
> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
> OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
> - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
> #endif
> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 0f480bec48d0..23eb18495a66 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -28,6 +28,7 @@
> #include "dpctl.h"
> #include "dpif-netdev.h"
> #include "flow.h"
> +#include "netdev-offload.h"
> #include "netdev-provider.h"
> #include "netdev.h"
> #include "netlink.h"
> @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
> }
>
> bool
> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
> +dpif_may_support_explicit_drop_action(const struct dpif *dpif)
> {
> -return dpif_is_netdev(dpif);
> +/* TC does not support offloading this action. */
> +return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
> }
>
> bool
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 0f2dc2ef3c55..a764e8a592bd 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -940,7 +940,7 @@ 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_supports_explicit_drop_action(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 c4e2e867ecdc..b854ba3636ed 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -221,6 +221,7 @@ 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)
> @@ -391,6 +392,10 @@ 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;
> @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
>
> shash_add(_dpif_backers, type, backer);
>
> +atomic_init(>rt_support.explicit_drop_action, false);
This initialization seems unnecessary since the field will be initialized
in the check_support() right after. And it also a bit out of place, since
we are not initializing anything else.
> check_support(backer);
> atomic_count_init(>tnl_count, 0);
>
> @@ -855,7 +861,12 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto)
> bool
> ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto)
> {
> -return ofproto->backer->rt_support.explicit_drop_action;
> +bool value;
> +
> +atomic_read_relaxed(>backer->rt_support.explicit_drop_action,
> +);
> +
May remove te empty lne here.
> +return value;
> }
>
> bool
> @@ -1379,6 +1390,39 @@ check_ct_timeout_policy(struct dpif_backer *backer)
> return !error;
> }
>
> +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action.
> */
> +static bool
> +check_drop_action(struct dpif_backer *backer)
> +{
> +struct odputil_keybuf keybuf;
> +uint8_t actbuf[NL_A_U32_SIZE];
> +struct ofpbuf actions;
> +struct ofpbuf key;
> +struct flow flow;
> +bool supported;
> +
> +
