Re: [PATCH net-next v2 1/2] ipv6: introduce and uses route look hints for list input

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Mon, Nov 18, 2019 at 4:59 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Mon, 2019-11-18 at 15:29 -0500, Willem de Bruijn wrote:
> > On Mon, Nov 18, 2019 at 6:03 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > When doing RX batch packet processing, we currently always repeat
> > > the route lookup for each ingress packet. If policy routing is
> > > configured, and IPV6_SUBTREES is disabled at build time, we
> > > know that packets with the same destination address will use
> > > the same dst.
> > >
> > > This change tries to avoid per packet route lookup caching
> > > the destination address of the latest successful lookup, and
> > > reusing it for the next packet when the above conditions are
> > > in place. Ingress traffic for most servers should fit.
> > >
> > > The measured performance delta under UDP flood vs a recvmmsg
> > > receiver is as follow:
> > >
> > > vanilla         patched         delta
> > > Kpps            Kpps            %
> > > 1431            1664            +14
> >
> > Since IPv4 speed-up is almost half and code considerably more complex,
> > maybe only do IPv6?
>
> uhmm... I would avoid that kind of assimmetry, and I would not look
> down on a 8% speedup, if possible.

Okay, that's fair.

> > > @@ -104,9 +127,18 @@ static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
> > >                 skb = l3mdev_ip6_rcv(skb);
> > >                 if (!skb)
> > >                         continue;
> > > -               ip6_rcv_finish_core(net, sk, skb);
> > > +               ip6_rcv_finish_core(net, sk, skb, hint);
> > >                 dst = skb_dst(skb);
> > >                 if (curr_dst != dst) {
> > > +                       if (ip6_can_cache_route_hint(net)) {
> > > +                               _hint.refdst = skb->_skb_refdst;
> > > +                               memcpy(&_hint.daddr, &ipv6_hdr(skb)->daddr,
> > > +                                      sizeof(_hint.daddr));
> > > +                               hint = &_hint;
> > > +                       } else {
> > > +                               hint = NULL;
> > > +                       }
> >
> > not needed. ip6_can_cache_route_hit is the same for all iterations of
> > the loop (indeed, compile time static), so if false, hint is never
> > set.
>
> I think this is needed, instead: if CONFIG_MULTIPLE_TABLES=y,
> fib6_has_custom_rules can change at runtime - from 'false' to 'true'.
> If we don't reset 'hint', we could end-up with use-after-free.

Uhm, of course, this is not compile time static at all. I clearly
missed a part.

But such a config change does not expect instantaneous effect on
packets in flight, like those in the recv rcu critical section? In
which case it should be safe to treat all skbs in the list the same.

I would need to read that code more closely to be certain, and the
current solution errs on the side of caution, so is definitely fine as
is, of course.