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 <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.
>
>> check_support(backer);
>> atomic_count_init(&backer->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(&ofproto->backer->rt_support.explicit_drop_action,
>> + &value);
>> +
>
> 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;
>> +
>> + 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_may_support_explicit_drop_action(backer->dpif) &&
>> + dpif_probe_feature(backer->dpif, "drop", &key, &actions,
>> NULL);
>
> Would be better if we probed actions with dpif_execute() instead
> of dpif_probe_feature(), since dpif_execute() doesn't interfere
> with the datapath, i.e. doesn't install actual flows.
>
> But I will not insist on that, since we do already have some
> actions probed this way. Maybe for a future larger cleanup.
Maybe add a match on some bogus dl_type like 0x1234 to the flow though,
so we're sure that it doesn't just drop all packets unconditionally
when installed. "flow.dl_type = htons(0x1234);" should be enough.
>
>> +
>> + VLOG_INFO("%s: Datapath %s explicit drop action",
>> + dpif_name(backer->dpif),
>> + (supported) ? "supports" : "does not support");
>> +
>> + return supported;
>> +}
>> +
>> /* 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,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 && ?
>
>> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action,
>> false);
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> static int
>> construct(struct ofproto *ofproto_)
>> {
>> @@ -5714,6 +5780,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);
>> @@ -5749,8 +5816,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");
>> @@ -6279,7 +6347,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")\
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
