Re: TCP and reordering

[Eric Dumazet <eric.dumazet@gmail.com>]

On Wed, 2012-11-28 at 15:47 +0000, David Woodhouse wrote:
> On Wed, 2012-11-28 at 04:52 -0800, Eric Dumazet wrote:
> > BQL is nice for high speed adapters.
> 
> For adapters with hugely deep queues, surely? There's a massive
> correlation between the two, of course — but PPP over L2TP or PPPoE
> ought to be included in the classification, right?
> 
> > For slow one, you always can stop the queue for each packet given to
> > start_xmit()
> > 
> > And restart the queue at TX completion.
> 
> Well yes, but only if we get notified of TX completion.
> 
> It's simple enough for the tty-based channels, and we can do it with a
> vcc->pop() function for PPPoATM. But for PPPoE and L2TP, how do we do
> it? We can install a skb destructor... but then we're stomping on TSQ's
> use of the destructor by orphaning it too soon.
> 
> I'm pondering something along the lines of
> 
> 	if (skb->destructor) {
> 		newskb = skb_clone(skb, GFP_KERNEL);
> 		if (newskb) {
> 	             skb_shinfo(newskb) = skb;
> 		     skb = newskb;
> 	        } 
> 	 }
> 	 skb_orphan(skb);
> 	 skb->destructor = ppp_chan_tx_completed;
> 
> 
> ... and then ppp_chan_tx_completed can also destroy the original skb
> (and hence invoke TSQ's destructor too) when the time comes. And in the
> (common?) case where we don't have an existing destructor, we don't
> bother with the skb_clone.
> 
> But I wish there was a nicer way to chain destructors. And no, I don't
> count what GSO does. We can't use the cb here anyway since we're passing
> it down the stack.
> 

My point was that if you limit number of in flight packet to 1,
its relatively easy to add glue in the priv dev data, so that you chain
the destructor without adding yet another fields in all skbs.

At start_xmit() do the following instead of skb_orphan(skb)

if (skb->destructor) {
     BUG_ON(priv->save_destructor);
     priv->save_destructor = skb->destructor;
     priv->save_sk = skb->sk;
     skb->sk = &priv->fake_sk;
}

skb->destructor = my_destructor;
/* stop the queue */
...
}


void my_destructor(struct sk_buff *skb)
{
    struct .... *priv = container_of(skb, struct ..., priv.fake_sk);

    skb->sk = skb->save_sk;
    priv->save_destructor(skb);
    priv->save_destructor = NULL;
    /* restart the queue */
    ...
}

Something like that.