On 15 Mar 2024, at 17:37, 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.
Yes, we might just remove hw_offload, and do something like:
(may_support) ? ", but not enabled due to a conflicting
configuration" : "");
But this might just raise the question, of what the conflicting configuration
might be ;)
>>> + 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" :
>>> "");
>>> +
>>> + 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.
In theory, unsetting it will keep active flows in TC, only new/updated flows
will be put in the kernel. So triggering the reconfigure in both will at least
solve some of the potential problems ;)
> 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
>
>>> + }
>>> +
>>> + 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
