Re: [PATCH 10/13] net: add paged frag destructor to skb_fill_page_desc()

["Michał Mirosław" <mirqus@gmail.com>]

W dniu 22 lipca 2011 23:07 użytkownik Ian Campbell
<Ian.Campbell@eu.citrix.com> napisał:
> On Fri, 2011-07-22 at 20:58 +0100, Michał Mirosław wrote:
>> 2011/7/22 Ian Campbell <ian.campbell@citrix.com>:
>> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> > index 9818fe2..faee8d3 100644
>> > --- a/include/linux/skbuff.h
>> > +++ b/include/linux/skbuff.h
>> > @@ -1134,12 +1134,14 @@ static inline int skb_pagelen(const struct sk_buff *skb)
>> >  * Does not take any additional reference on the fragment.
>> >  */
>> >  static inline void __skb_fill_page_desc(struct sk_buff *skb, int i,
>> > -                                       struct page *page, int off, int size)
>> > +                                       struct page *page,
>> > +                                       struct skb_frag_destructor *destroy,
>> > +                                       int off, int size)
>> >  {
>> >        skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
>> >
>> >        frag->page.p              = page;
>> > -       frag->page.destructor     = NULL;
>> > +       frag->page.destructor     = destroy;
>> >        frag->page_offset         = off;
>> >        frag->size                = size;
>> >  }
>>
>> You could just rename this function to e.g.
>> __skb_fill_fragile_page_desc() (or whatever name) add an inline
>> wrapper calling it with destroy == NULL. This will avoid touching all
>> those drivers which won't ever need this functionality.
>
> I could call this variant __skb_frag_init (which I think better fits
> into the pattern of the new functions) and leave the existing
> __skb_fill_page_desc as a compat wrapper if that's preferred but I was
> trying to avoid duplicating up constructors just for different sets of
> defaults.

It's just Huffman coding: since most users need destroy = NULL, it's
good to have a wrapper for this case as it will then take less time to
write and understand the code (you won't need to think what is this
NULL for in all those places).

Best Regards,
Michał Mirosław