On 25/02/2022 13:20, Eelco Chaudron wrote: > CAUTION: This message has originated from an External Source. Please use > proper judgment and caution when opening attachments, clicking links, or > responding to this email. > > > On 25 Feb 2022, at 13:57, Pieter Jansen van Vuuren wrote: > >> On 25/02/2022 10:08, Simon Horman wrote: >>> Hi Pieter, >>> >>> nice to hear from you :) >> >> Hi Simon. Thank you for the insightful feedback. >> >>> >>> On Tue, Feb 22, 2022 at 12:17:40PM +0000, [email protected] >>> wrote: >>>> From: Pieter Jansen van Vuuren <[email protected]> >>>> >>>> Previously when making use of the tc datapath to achieve decrement ttl, >>>> we would install a filter that matches on the ttl/hoplimit field and use >>>> a pedit set action to set the ttl/hoplimit to one less than the match >>>> value. This results in a filter for each unique ttl value we want to >>>> decrement. >>>> >>>> This patch introduces true decrement ttl which makes use of the pedit add >>>> action. Adding 0xff is equivalent to decrementing the ttl/hoplimit >>>> field. This also improves the hardware offload case by reducing the >>>> number of filters required to support decrement ttl. In order to utilise >>>> this, the following config option needs to be set to true. >>> >>> I'd be interested to understand the specific motivation for wanting >>> to reduce flows. But I agree that in the general case it seems nice. >> >> I think main motivation is just relaxing some of the constraints on HW >> offload slightly; although matching ip ttl and setting it to one less >> than the match field is possible, it feels a bit wasteful if we really >> only decrement the field. I concede that depending on the use case the >> gain might be insignificant. Mostly I just wanted to start a discussion >> on this as I think we can improve this area a bit. >> >>> >>> One thing that I am curious about is the approach you have taken, >>> which seems to be to patch pedit commands. But I am wondering if you >>> considered plumbing the OpenFlow DEC_TTL command more directly into >>> the ODP layer - perhaps hinging on the handling of OFPACT_DEC_TTL in >>> do_xlate_actions(). >> >> Yes, I think that is good point. I think this ties in with Eelco's work; >> https://patchwork.ozlabs.org/project/openvswitch/patch/162134902591.762107.15543938565196336653.st...@wsfd-netdev64.ntdv.lab.eng.bos.redhat.com/ >> >> Possibly this patch needs to be rebased on top Eelco's v5 of the above. >> >>> >>>> ovs-vsctl set Open_vSwitch . other_config:tc-pedit-add=true >>> >>>> Signed-off-by: Pieter Jansen van Vuuren <[email protected]> >>>> Reviewed-by: Alejandro Lucero Palau <[email protected]> >>>> Reviewed-by: Martin Habets <[email protected]> >>>> --- >>>> .../linux/compat/include/linux/openvswitch.h | 1 + >>>> lib/dpif-netdev.c | 1 + >>>> lib/dpif.c | 1 + >>>> lib/netdev-offload-tc.c | 16 ++++++++- >>>> lib/netdev-offload.c | 12 +++++++ >>>> lib/netdev-offload.h | 1 + >>>> lib/odp-execute.c | 2 ++ >>>> lib/odp-util.c | 4 +++ >>>> lib/tc.c | 35 +++++++++++++++++-- >>>> lib/tc.h | 1 + >>>> ofproto/ofproto-dpif-ipfix.c | 1 + >>>> ofproto/ofproto-dpif-sflow.c | 1 + >>>> vswitchd/vswitch.xml | 15 ++++++++ >>>> 13 files changed, 87 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h >>>> b/datapath/linux/compat/include/linux/openvswitch.h >>>> index 8d9300091..f7daffeb0 100644 >>>> --- a/datapath/linux/compat/include/linux/openvswitch.h >>>> +++ b/datapath/linux/compat/include/linux/openvswitch.h >>>> @@ -1075,6 +1075,7 @@ enum ovs_action_attr { >>>> OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ >>>> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */ >>>> #endif >>>> + OVS_ACTION_ATTR_DEC_TTL, /* No argument. */ >>> >>> I'm not entirely sure, but I think that: >>> * if this attribute is not used by the OVS kernel datapath then >>> it should be inside the ifndef; otherwise >>> * it should be above the ifndef >>> >>> Perhaps there is precedence, but it would strike me as unusual to implement >>> a feature in the TC offload datapath but not in the OVS kernel datapath. >> >> Yeah, I see. I think you are correct; this needs to be implement in the >> kernel >> datapath as well and likely before we tackle the TC offload datapath. > > The kernel datapath (meaning the kernel module) already has the > implementation. > >> I am hoping to rebase this on Eelco's patch that deals with this. > > The problem with the current v5(v4) patchset is some architectural things > that need to be solved, which might make the actual benefits of having it > offloaded not matter. I guess it depends on the actual use case, which was > not clarified by Nokia. > > See the following email: > https://mail.openvswitch.org/pipermail/ovs-dev/2021-July/386296.html > Especially the part under “I was preparing my v5, and I noticed that a bunch > of self-tests fail. I was wondering why I (and Matteo/Bindiya) never noticed. > For me, it was because I was running make check on my dev system, which had > an old kernel :( The datapath tests I was running on my DUT.” > > > //Eelco
I see, thank you Eelco for the clarification. So from the thread I gather that there are at least 3 issues (corner cases); 1. dec_ttl being non-reversible, solved with the clone action (in OVS datapath). As you pointed out this in not HW offloadable and will not be of much use for TC datapath. 2. encapsulation that copies the ttl field, which when using dec_ttl is set to the incorrect value. Is this value "correct_ttl-1" or is it completely missing as in it is "0"? 3. dec_ttl that stops processing the action list once hitting the exception case. Possibly solved with the clone action, similar to (1). Is my understanding here correct? It does seem non-trivial and I agree that this warrants some more investigation. Thanks again for the insight here. > >>> <snip> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
