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.
>> 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
