On Mon, 02 Oct 2017 05:15:32 -0000
Michael Witten <mfwit...@gmail.com> wrote:
> On Sun, 1 Oct 2017 17:59:09 -0700, Stephen Hemminger wrote:
>
> > On Sun, 01 Oct 2017 22:19:20 -0000 Michael Witten wrote:
> >
> >> + spin_lock_irqsave(&q->lock, flags);
> >> + skb = q->next;
> >> + __skb_queue_head_init(q);
> >> + spin_unlock_irqrestore(&q->lock, flags);
> >
> > Other code manipulating lists uses splice operation and
> > a sk_buff_head temporary on the stack. That would be easier
> > to understand.
> >
> > struct sk_buf_head head;
> >
> > __skb_queue_head_init(&head);
> > spin_lock_irqsave(&q->lock, flags);
> > skb_queue_splice_init(q, &head);
> > spin_unlock_irqrestore(&q->lock, flags);
> >
> >
> >> + while (skb != head) {
> >> + next = skb->next;
> >> kfree_skb(skb);
> >> + skb = next;
> >> + }
> >
> > It would be cleaner if you could use
> > skb_queue_walk_safe rather than open coding the loop.
> >
> > skb_queue_walk_safe(&head, skb, tmp)
> > kfree_skb(skb);
>
> I appreciate abstraction as much as anybody, but I do not believe
> that such abstractions would actually be an improvement here.
>
> * Splice-initing seems more like an idiom than an abstraction;
> at first blush, it wouldn't be clear to me what the intention
> is.
>
> * Such abstractions are fairly unnecessary.
>
> * The function as written is already so short as to be
> easily digested.
>
> * More to the point, this function is not some generic,
> higher-level algorithm that just happens to employ the
> socket buffer interface; rather, it is a function that
> implements part of that very interface, and may thus
> twiddle the intimate bits of these data structures
> without being accused of abusing a leaky abstraction.
>
> * Such abstractions add overhead, if only conceptually. In this
> case, a temporary socket buffer queue allocates *3* unnecessary
> struct members, including a whole `spinlock_t' member:
>
> prev
> qlen
> lock
>
> It's possible that the compiler will be smart enough to leave
> those out, but I have my suspicions that it won't, not only
> given that the interface contract requires that the temporary
> socket buffer queue be properly initialized before use, but
> also because splicing into the temporary will manipulate its
> `qlen'. Yet, why worry whether optimization happens? The whole
> issue can simply be avoided by exploiting the intimate details
> that are already philosophically available to us.
>
> Similarly, the function `skb_queue_walk_safe' is nice, but it
> loses value both because a temporary queue loses value (as just
> described), and because it ignores the fact that legitimate
> access to the internals of these data structures allows for
> setting up the requested loop in advance; that is to say, the
> two parts of the function that we are now debating can be woven
> together more tightly than `skb_queue_walk_safe' allows.
>
> For these reasons, I stand by the way that the patch currently
> implements this function; it does exactly what is desired, no more
> or less.
>
> Sincerely,
> Michael Witten
The point is that there was discussion in the past of replacing
the next/prev as used in skb with more generic code from list.h.
If the abstraction was used, then this code would just work.
The temporary skb_buff_head is on the stack, and any
access to updating those fields like qlen are in CPU cache
and therefore have very little impact on any peformance.
