Re: [RFC PATCH] ip: re-introduce fragments cache worker

[Paolo Abeni <pabeni@redhat.com>]

On Fri, 2018-07-20 at 08:58 -0700, Eric Dumazet wrote:
> On Fri, Jul 20, 2018 at 7:48 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > Hi,
> > 
> > On Mon, 2018-07-09 at 05:50 -0700, Eric Dumazet wrote:
> > > On 07/09/2018 04:39 AM, Eric Dumazet wrote:
> > > 
> > > > Alternatively, you could try to patch fq_codel to drop all frags of one UDP datagram
> > > > instead of few of them.
> > > 
> > > A first step would be to make sure fq_codel_hash() (using skb_get_hash(skb)) selects
> > > the same bucket for all frags of a datagram :/
> > 
> > I gave the above a shot and I have some non upstream ready but somewhat
> > working code. Anyway it has some issues I'm unable to solve:
> > * it's very invasive for fq_codel, because I need to parse each packet
> > looking for the fragment id
> > * the parsing overhead can't be easily avoided for non fragments
> 
> Have you tried using ip_defrag(net, skb,  IP_DEFRAG_QDISC) from fq_codel ?
> (adding a new value in ip_defrag_users enum)
> 
> if (skb->protocol == htons(ETH_P_IP) {
>       if (ip_is_fragment(ip_hdr(skb))) {
>            if ((ip_defrag(net, skb, IP_DEFRAG_QDISC))
>                 return 0;
> ...

Thank you for the feedback. I must admit this quite in the opposite
direction of what I have attempted so far. I'll try that.
Thanks.
Still for ipv6 it will require a litte more work inside fq_codel.

> > I tried also something hopefully along the same lines of your other
> > suggestion (drop eariler the fragment queues when above low threshold):
> > when allocating a new frag queue and the ipfrag mem is above the low
> > th, another frag queue is selected in a pseudorandom way and dropped.
> 
> The problem with any strategy like that, is that forthcoming fragments
> for this frag queue
> will create another frag queue, that will never have a chance to complete.
> 
> Some workloads might benefit, others might not.

Yes, of course: is an heuristic, but is cheap code-wise, and it can be
disabled setting low th >= high th, so that the kernel will behave
exactly as it does now, and the kind of workloads we could cope with
will increase without adding new knobs.

Cheers,

Paolo