On 15 Mar 2024, at 18:59, Eric Garver wrote:
> On Fri, Mar 15, 2024 at 12:37:54PM -0400, Eric Garver wrote:
>> On Fri, Mar 15, 2024 at 04:55:50PM +0100, Eelco Chaudron wrote:
>>>
>>>
>>> On 11 Mar 2024, at 18:51, Eric Garver wrote:
>>>
>>>> Kernel support has been added for this action. As such, we need to probe
>>>> the datapath for support.
>>>>
>>>> Signed-off-by: Eric Garver <e...@garver.life>
>>>
>>> Thanks for submitting the changes requested. Some comments below.
>>>
>>> Cheers,
>>>
>>> Eelco
>>>
>>>
>>>> ---
>>>> include/linux/openvswitch.h | 2 +-
>>>> lib/dpif.c | 6 ++-
>>>> lib/dpif.h | 2 +-
>>>> ofproto/ofproto-dpif.c | 77 ++++++++++++++++++++++++++++++++++---
>>>> ofproto/ofproto-dpif.h | 4 +-
>>>> 5 files changed, 81 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 2ec1cd1854ab..7bacd2120c78 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);
>>>> check_support(backer);
>>>> atomic_count_init(&backer->tnl_count, 0);
>>>>
>>>> @@ -855,7 +861,10 @@ 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;
>>>
>>> nit: I would add a new line here.
>>
>> ACK.
>>
>>>> + atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action,
>>>> + &value);
>>>> + return value;
>>>> }
>>>>
>>>> bool
>>>> @@ -1379,6 +1388,41 @@ 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;
>>>> + bool hw_offload;
>>>> +
>>>> + struct odp_flow_key_parms odp_parms = {
>>>> + .flow = &flow,
>>>> + .probe = true,
>>>> + };
>>>> +
>>>> + memset(&flow, 0, sizeof flow);
>>>> + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
>>>> + odp_flow_key_from_flow(&odp_parms, &key);
>>>> +
>>>> + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf);
>>>> + nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK);
>>>> +
>>>> + supported = dpif_probe_feature(backer->dpif, "drop", &key, &actions,
>>>> NULL);
>>>> + hw_offload = !dpif_may_support_explicit_drop_action(backer->dpif);
>>>
>>> The use case of this function seems odd here. This since
>>> dpif_may_support_explicit_drop_action() doesn't imply that we're
>>> specifically checking for HW offload being the issue. If someone updates
>>> the function with another reason for it not being supported we will break
>>> the implementation here. What do you think?
>>
>> The reason it was changed to user
>> dpif_may_support_explicit_drop_action() was because it was suggested to
>> avoid calling dpif_is_netdev() from ofproto. I assume it's similar for
>> netdev_is_flow_api_enabled().
>>
>> "hw_offload" is probably a bad variable name. Maybe we completely avoid
>> mentioning hw_offload here and in the log. Instead we do:
>>
>> + supported = dpif_may_support_explicit_drop_action(backer->dpif)
>> && dpif_probe_feature(backer->dpif, "drop", &key, &actions,
>> NULL);
>>
>>
>> Is this agreeable?
>>
>>>> +
>>>> + VLOG_INFO("%s: Datapath %s explicit drop action%s",
>>>> + dpif_name(backer->dpif),
>>>> + (supported) ? "supports" : "does not support",
>>>> + (hw_offload) ? ", but not enabled due to hardware offload"
>>>> : "");
>>
>> We would simply remove the log mentioning hardware offload.
>>
>>>> +
>>>> + return supported && !hw_offload;
>>>> +}
>>>> +
>>>> /* Tests whether 'backer''s datapath supports the all-zero SNAT case. */
>>>> static bool
>>>> dpif_supports_ct_zero_snat(struct dpif_backer *backer)
>>>> @@ -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,27 @@ check_support(struct dpif_backer *backer)
>>>> backer->rt_support.odp.nd_ext = check_nd_extensions(backer);
>>>> }
>>>>
>>>> +/* For TC, if vswitchd started with other_config:hw-offload set to
>>>> "false", and
>>>> + * the configuration is now "true", then we need to re-probe datapath
>>>> support
>>>> + * for the "drop" action. */
>>>> +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)) {
>>>> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action,
>>>> false);
>>>> + return true;
>>>
>>> This should probably also work in the reverse direction. If it´s enabled on
>>> startup, but now disabled?
>>
>> I'll make the change.
>>
>> IIRC, unsetting "hw-offload" requires a restart. Setting it does not.
>> Even though it's documented that a restart is required.
>>
>> other_config : hw-offload: optional string, either true or false
>> Set this value to true to enable netdev flow offload.
>>
>> The default value is false. Changing this value
>> requires
>> restarting the daemon
>> [..]
>>
>> https://www.openvswitch.org/support/dist-docs/ovs-vswitchd.conf.db.5.html
>
> Testing and code inspection, i.e. netdev_set_flow_api_enabled(), confirm
> that we don't support setting hw-offload=false without a restart. We
> only support false --> true at runtime.
>
> As such, I won't add support for this here.
ACK, was not aware of this... Thanks, no I know ;)
>>>> + }
>>>> +
>>>> + return false;
>>>> +}
>>>> +
>>>> static int
>>>> construct(struct ofproto *ofproto_)
>>>> {
>>>> @@ -5708,6 +5773,7 @@ ct_zone_limit_protection_update(const char
>>>> *datapath_type, bool protected)
>>>> static void
>>>> get_datapath_cap(const char *datapath_type, struct smap *cap)
>>>> {
>>>> + bool explicit_drop_action;
>>>> struct dpif_backer_support *s;
>>>> struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
>>>> datapath_type);
>>>> @@ -5743,8 +5809,9 @@ get_datapath_cap(const char *datapath_type, struct
>>>> smap *cap)
>>>> smap_add_format(cap, "max_hash_alg", "%"PRIuSIZE, s->max_hash_alg);
>>>> smap_add(cap, "check_pkt_len", s->check_pkt_len ? "true" : "false");
>>>> smap_add(cap, "ct_timeout", s->ct_timeout ? "true" : "false");
>>>> + atomic_read_relaxed(&s->explicit_drop_action, &explicit_drop_action);
>>>> smap_add(cap, "explicit_drop_action",
>>>> - s->explicit_drop_action ? "true" :"false");
>>>> + explicit_drop_action ? "true" :"false");
>>>> smap_add(cap, "lb_output_action", s->lb_output_action ? "true" :
>>>> "false");
>>>> smap_add(cap, "ct_zero_snat", s->ct_zero_snat ? "true" : "false");
>>>> smap_add(cap, "add_mpls", s->add_mpls ? "true" : "false");
>>>> @@ -6273,7 +6340,7 @@ show_dp_feature_bool(struct ds *ds, const char
>>>> *feature, const bool *b)
>>>> ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No");
>>>> }
>>>>
>>>> -static void OVS_UNUSED
>>>> +static void
>>>> show_dp_feature_atomic_bool(struct ds *ds, const char *feature,
>>>> const atomic_bool *b)
>>>> {
>>>> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
>>>> index 92d33aa64709..d33f73df8aed 100644
>>>> --- a/ofproto/ofproto-dpif.h
>>>> +++ b/ofproto/ofproto-dpif.h
>>>> @@ -51,6 +51,7 @@
>>>> #include "hmapx.h"
>>>> #include "odp-util.h"
>>>> #include "id-pool.h"
>>>> +#include "ovs-atomic.h"
>>>> #include "ovs-thread.h"
>>>> #include "ofproto-provider.h"
>>>> #include "util.h"
>>>> @@ -202,7 +203,8 @@ struct group_dpif *group_dpif_lookup(struct
>>>> ofproto_dpif *,
>>>> DPIF_SUPPORT_FIELD(bool, ct_timeout, "Conntrack timeout policy")
>>>> \
>>>>
>>>> \
>>>> /* True if the datapath supports explicit drop action. */
>>>> \
>>>> - DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop
>>>> action") \
>>>> + DPIF_SUPPORT_FIELD(atomic_bool, explicit_drop_action,
>>>> \
>>>> + "Explicit Drop action")
>>>> \
>>>>
>>>> \
>>>> /* True if the datapath supports balance_tcp optimization */
>>>> \
>>>> DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP
>>>> mode")\
>>>> --
>>>> 2.43.0
>>>>
>>>> _______________________________________________
>>>> 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
>>
>> _______________________________________________
>> 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
