On Fri, Nov 13, 2020 at 4:06 AM Ilya Maximets <i.maxim...@ovn.org> wrote: > > On 11/13/20 11:04 AM, Eelco Chaudron wrote: > > Add support for the dec_ttl action. Instead of programming the datapath with > > a flow that matches the packet TTL and an IP set, use a single dec_ttl > > action. > > > > The old behavior is kept if the new action is not supported by the datapath. > > > > # ovs-ofctl dump-flows br0 > > cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392, ip > > actions=dec_ttl,NORMAL > > cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168, > > actions=NORMAL > > > > # ping -c1 -t 20 192.168.0.2 > > PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data. > > IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP (1), > > length 84) > > 192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1, length > > 64 > > > > Linux netlink datapath support depends on upstream Linux commit: > > 744676e77720 ("openvswitch: add TTL decrement action") > > > > > > Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has been > > defined, and to make sure the IDs are in sync, it had to be added to the > > OVS source tree. This required some additional case statements, which > > should be revisited once the OVS implementation is added. > > > > > > Co-developed-by: Matteo Croce <mcr...@linux.microsoft.com> > > Co-developed-by: Bindiya Kurle <bindiyaku...@gmail.com> > > Signed-off-by: Eelco Chaudron <echau...@redhat.com> > > > > --- > > v2: - Used definition instead of numeric value in format_dec_ttl_action() > > - Changed format from "dec_ttl(ttl<=1(<actions>)) to > > "dec_ttl(le_1(<actions>))" to be more in line with the check_pkt_len > > action. > > - Fixed parsing of "dec_ttl()" action for adding a dp flow. > > - Cleaned up format_dec_ttl_action() > > > > datapath/linux/compat/include/linux/openvswitch.h | 8 ++ > > lib/dpif-netdev.c | 4 + > > lib/dpif.c | 4 + > > lib/odp-execute.c | 102 > > ++++++++++++++++++++- > > lib/odp-execute.h | 2 > > lib/odp-util.c | 42 +++++++++ > > lib/packets.h | 13 ++- > > ofproto/ofproto-dpif-ipfix.c | 2 > > ofproto/ofproto-dpif-sflow.c | 2 > > ofproto/ofproto-dpif-xlate.c | 54 +++++++++-- > > ofproto/ofproto-dpif.c | 37 ++++++++ > > ofproto/ofproto-dpif.h | 6 + > > 12 files changed, 253 insertions(+), 23 deletions(-) > > > > <snip> > > > +static void > > +format_dec_ttl_action(struct ds *ds,const struct nlattr *attr, > > + const struct hmap *portno_names) > > +{ > > + const struct nlattr *nla_acts = nl_attr_get(attr); > > + int len = nl_attr_get_size(attr); > > + > > + ds_put_cstr(ds,"dec_ttl(le_1("); > > + > > + if (len > 4 && nla_acts->nla_type == OVS_DEC_TTL_ATTR_ACTION) { > > + /* Linux kernel add an additional envelope we should strip. */ > > + len -= nl_attr_len_pad(nla_acts, len); > > + nla_acts = nl_attr_next(nla_acts); > > CC: Pravin > > I looked at the kernel and I agree that there is a clear bug in kernel's > implementaion of this action. It receives messages on format: > OVS_ACTION_ATTR_DEC_TTL(<list of actions>), > but reports back in format: > OVS_ACTION_ATTR_DEC_TTL(OVS_DEC_TTL_ATTR_ACTION(<list of actions>)). > > Since 'OVS_DEC_TTL_ATTR_ACTION' exists, it's clear that original design > was to have it, i.e. the correct format should be the form that > kernel reports back to userspace. I'd guess that there was a plan to > add more features to OVS_ACTION_ATTR_DEC_TTL in the future, e.g. set > actions execution threshold to something different than 1, so it make > some sense. > > Anyway, the bug is in the kernel part of parsing the netlink message and > it should be fixed. What I'm suggesting is to send a bugfix to kernel > to accept only format with nested OVS_DEC_TTL_ATTR_ACTION. Since this > feature was never implemented in userspace OVS, this change will not > break anything. On the OVS side we should always format netlink messages > in a correct way. We have a code that checks feature existence in kernel > and it should fail if kernel is broken (as it is right now). In this case > OVS will continue to use old implementation with setting the ttl field. > > Since the patch for a kernel is a bug fix, it should be likely backported > to stable versions, and distributions will pick it up too. > > Thoughts? >
Yes, I agree. lets fix the interface to use currect nested netlink attribute to pass the actions Thanks. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev