On 5 Apr 2024, at 15:43, Ilya Maximets wrote:
> On 4/5/24 15:08, Ilya Maximets wrote:
>> On 4/5/24 15:01, Eric Garver wrote:
>>> On Fri, Apr 05, 2024 at 11:39:58AM +0200, Eelco Chaudron wrote:
>>>>
>>>>
>>>> On 3 Apr 2024, at 16:35, 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>
>>>>> ---
>>>>> 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 d18754c84f62..d9fb991ef234 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..32d037be607c 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;
>>>>> @@ -855,7 +860,11 @@ 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);
>>>>> + return value;
>>>>> }
>>>>>
>>>>> bool
>>>>> @@ -1379,6 +1388,40 @@ 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;
>>>>> + bool supported;
>>>>> +
>>>>> + struct flow flow = {
>>>>> + .dl_type = CONSTANT_HTONS(0x1234), /* bogus */
>>>>> + };
>>>>> + struct odp_flow_key_parms odp_parms = {
>>>>> + .flow = &flow,
>>>>> + .probe = true,
>>>>> + };
>>>>> +
>>>>> + 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);
>>>>> +
>>>>> + 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 +1692,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 +1710,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)) {
>>>>> + ovs_assert(!check_drop_action(backer));
>>>>
>>>> If we only call this for the log message, would just adding the log
>>>> message here not be better?
>>>> It avoids the installation of a flow,
>>
>> We're not going to install the flow, because there is a check for
>> dpif_may_support_explicit_drop_action() in the check_drop_action().
>>>> and I have not thought this through, but it might confuse the revalidator.
>>
>> So, the revalidator will not be confused.
>>
>>>
>>> So you and Ilya would rather I insert a VLOG_INFO() here? That's fine
>>> with me.
>>
>> I'd keep as is, unless Eelco has a strong opinion towards the log.
>>
>>>
>>> Can we agree on the message before I post another revision?
>>>
>>> VLOG_INFO("%s: Datapath supports explicit drop action, "
>>> "but disabled due to hw-offload=true.",
>>> dpif_name(backer->dpif))
>>>
>>> Is that agreeable?
>
> In case Eelco prefers a solution with the log, I'd suggest to
> just log "%s: Datapath no longer supports explicit drop action".
> The reasoning is the same as before, this module doesn't know
> why exactly dpif_may_support_explicit_drop_action() now returns
> 'false' and shouldn't make assumptions that it is because of
> hardware offload. And it should be simple enough to correlate
> this message with enabling of the hardware offload.
I once again forgot this is for enabling TC only (not disabling), so let’s keep
the patch as is.
Acked-by: Eelco Chaudron <echau...@redhat.com>
Cheers,
Eelco
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
