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 <echau...@redhat.com>
>>> Signed-off-by: Eric Garver <e...@garver.life>
>>> ---
>>> 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(&all_dpif_backers, type, backer);
>>>
>>> + atomic_init(&backer->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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
