On 7/7/23 14:11, Eelco Chaudron wrote:
>
>
> On 7 Jul 2023, at 13:03, Ilya Maximets wrote:
>
>> On 7/6/23 11:30, Eelco Chaudron wrote:
>>>
>>>
>>> On 30 Jun 2023, at 21:05, Eric Garver wrote:
>>>
>>> Hi Eric,
>>>
>>> I guess some description here would be good. Maybe add some info that this
>>> is added due to kernel support being added.
>>>
>>>
>>> In general, the patch looks good, and I would ack it. However, there is a
>>> potential issue with TC hardware offload. If the patch gets applied as is,
>>> and the kernel fix gets in, the additional DROP actions get added, but they
>>> will not be offloaded to hardware and are only visible as kernel dp entries.
>>>
>>> I guess this might not be a real problem. However, people who upgrade OVS
>>> with tc offload might get confused why they now get these drop rules in the
>>> kernel dp. To solve this, we could simply skip adding them if hardware
>>> offload is enabled. So the behavior stays the same. Something like this
>>> (not tested, and we probably need to add dpif_is_netlink()):
>>>
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 29f4daa63..bc39e45bd 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -34,12 +34,14 @@
>>> #include "csum.h"
>>> #include "dp-packet.h"
>>> #include "dpif.h"
>>> +#include "dpif-netdev.h"
>>> #include "in-band.h"
>>> #include "lacp.h"
>>> #include "learn.h"
>>> #include "mac-learning.h"
>>> #include "mcast-snooping.h"
>>> #include "multipath.h"
>>> +#include "netdev-offload.h"
>>> #include "netdev-vport.h"
>>> #include "netlink.h"
>>> #include "nx-match.h"
>>> @@ -8199,7 +8201,8 @@ exit:
>>>
>>> /* Install drop action if datapath supports explicit drop action. */
>>> if (xin->odp_actions && !xin->odp_actions->size &&
>>> - ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
>>> + ovs_explicit_drop_action_supported(ctx.xbridge->ofproto) &&
>>> + /* Do not add drop actions if TC hardware offload is enabled, as
>>> + * as TC does not support offloading the drop action. */
>>> + (!netdev_is_flow_api_enabled() || dpif_is_netdev(xbridge->dpif))) {
>>
>> I'd argue that the check should not be here. We should not set the
>> baker.rt_support flag in the first place instead.
>
> Yes, that was my initial thought also, but what if hw offload gets enabled
> later? We might need some re-evaluation at that stage. I know the docs say
> you have to restart ovs when you enable HW offload, but that is not always
> happening in practice.
But if we keep it this way we'll have misleading information that it is
supported in the log, while it will not be used in practice.
>
>>> put_drop_action(xin->odp_actions, ctx.error);
>>> }
>>>
>>> I do not see a way for TC to support this as is in the kernel. We probably
>>> need some new kind of drop reason supported TC action.
>>>
>>> I know other people are exploring to see if this explicit drop action can
>>> be used for other, more user-level things. If this is true this TC part
>>> needs some proper fixing or we end up with rules not being offloaded.
>>>
>>> Added some more people, who might have some opinions on TC offload / ideas
>>> on how to add SKB_DROP_REASON support to TC.
>>>
>>> Cheers,
>>>
>>>
>>> Eelco
>>>
>>>> Signed-off-by: Eric Garver <e...@garver.life>
>>>> ---
>>>> include/linux/openvswitch.h | 2 +-
>>>> lib/dpif.c | 6 ------
>>>> lib/dpif.h | 1 -
>>>> ofproto/ofproto-dpif.c | 34 ++++++++++++++++++++++++++++++++--
>>>> 4 files changed, 33 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 d328bf288de0..dc02ec912693 100644
>>>> --- a/lib/dpif.c
>>>> +++ b/lib/dpif.c
>>>> @@ -1928,12 +1928,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
>>>> return dpif_is_netdev(dpif);
>>>> }
>>>>
>>>> -bool
>>>> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
>>>> -{
>>>> - return dpif_is_netdev(dpif);
>>>> -}
>>>> -
>>>> bool
>>>> dpif_supports_lb_output_action(const struct dpif *dpif)
>>>> {
>>>> diff --git a/lib/dpif.h b/lib/dpif.h
>>>> index 129cbf6a1d5f..fc1719f88b4f 100644
>>>> --- a/lib/dpif.h
>>>> +++ b/lib/dpif.h
>>>> @@ -938,7 +938,6 @@ 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_synced_dp_layers(struct dpif *);
>>>>
>>>> /* Log functions. */
>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>> index fad7342b0b02..c490a3c1da48 100644
>>>> --- a/ofproto/ofproto-dpif.c
>>>> +++ b/ofproto/ofproto-dpif.c
>>>> @@ -1375,6 +1375,37 @@ 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_probe_feature(backer->dpif, "drop", &key, &actions,
>>>> NULL);
>>>> +
>>>> + VLOG_INFO("%s: Datapath %s 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)
>>>> @@ -1627,8 +1658,7 @@ 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);
>>>> + 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);
>>>> --
>>>> 2.39.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
