lkml.org 
[lkml]   [2019]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next] openvswitch: add TTL decrement action
On Tue, Nov 12, 2019 at 04:46:12PM +0100, Matteo Croce wrote:
> On Tue, Nov 12, 2019 at 4:00 PM Simon Horman <simon.horman@netronome.com> wrote:
> >
> > On Tue, Nov 12, 2019 at 11:25:18AM +0100, Matteo Croce wrote:
> > > New action to decrement TTL instead of setting it to a fixed value.
> > > This action will decrement the TTL and, in case of expired TTL, send the
> > > packet to userspace via output_userspace() to take care of it.
> > >
> > > Supports both IPv4 and IPv6 via the ttl and hop_limit fields, respectively.
> > >
> >
> > Usually OVS achieves this behaviour by matching on the TTL and
> > setting it to the desired value, pre-calculated as TTL -1.
> > With that in mind could you explain the motivation for this
> > change?
> >
>
> Hi,
>
> the problem is that OVS creates a flow for each ttl it see. I can let
> vswitchd create 255 flows with like this:
>
> $ for i in {2..255}; do ping 192.168.0.2 -t $i -c1 -w1 &>/dev/null & done
> $ ovs-dpctl dump-flows |fgrep -c 'set(ipv4(ttl'
> 255

Hi,

so the motivation is to reduce the number of megaflows in the case
where flows otherwise match but the TTL differs. I think this makes sense
and the absence of this feature may date back to designs made before
megaflow support was added - just guessing.

I think this is a reasonable feature but I think it would be good
to explain the motivation in the changelog.

> > > @@ -1174,6 +1174,43 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb,
> > > nla_len(actions), last, clone_flow_key);
> > > }
> > >
> > > +static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
> > > +{
> > > + int err;
> > > +
> > > + if (skb->protocol == htons(ETH_P_IPV6)) {
> > > + struct ipv6hdr *nh = ipv6_hdr(skb);
> > > +
> > > + err = skb_ensure_writable(skb, skb_network_offset(skb) +
> > > + sizeof(*nh));
> > > + if (unlikely(err))
> > > + return err;
> > > +
> > > + if (nh->hop_limit <= 1)
> > > + return -EHOSTUNREACH;
> > > +
> > > + key->ip.ttl = --nh->hop_limit;
> > > + } else {
> > > + struct iphdr *nh = ip_hdr(skb);
> > > + u8 old_ttl;
> > > +
> > > + err = skb_ensure_writable(skb, skb_network_offset(skb) +
> > > + sizeof(*nh));
> > > + if (unlikely(err))
> > > + return err;
> > > +
> > > + if (nh->ttl <= 1)
> > > + return -EHOSTUNREACH;
> > > +
> > > + old_ttl = nh->ttl--;
> > > + csum_replace2(&nh->check, htons(old_ttl << 8),
> > > + htons(nh->ttl << 8));
> > > + key->ip.ttl = nh->ttl;
> > > + }
> >
> > The above may send packets with TTL = 0, is that desired?
> >
>
> If TTL is 1 or 0, execute_dec_ttl() returns -EHOSTUNREACH, and the
> caller will just send the packet to userspace and then free it.
> I think this is enough, am I missing something?

No, you are not. I was missing something.
I now think this logic is fine.

\
 
 \ /
  Last update: 2019-11-13 07:58    [W:0.072 / U:2.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site