Messages in this thread | | | Subject | Re: [PATCH] mm: add a "struct page_frag" type containing a page, offset and length | From | Ian Campbell <> | Date | Fri, 14 Oct 2011 07:56:02 +0100 |
| |
On Thu, 2011-10-13 at 22:22 +0100, Andrew Morton wrote: > Looks OK to me. I'm surprised we don't already have such a thing. > > Review comments: > > > > +struct page_frag { > > + struct page *page; > > +#if (BITS_PER_LONG > 32) || (PAGE_SIZE >= 65536) > > It does add risk that people will add compile warnings and bugs by > failing to consider or test the other case. > > We could reduce that risk by doing > > #if (PAGE_SIZE >= 65536) > > but then the 32-bit version would hardly ever be tested at all.
Indeed. The first variant has the benefit that most 32-bit arches will test one case and most 64-bit ones the other.
Perhaps the need to keep this struct small is not so acute as it is for the skb_frag_t I nicked it from and just using __u32 unconditionally is sufficient?
> > > + __u32 page_offset; > > I suggest this be called simply "offset".
ACK.
Thanks,
Ian.
| |