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.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
