Re: [PATCH] mm: add a "struct page_frag" type containing a page,offset and length

[Andrew Morton]

On Thu, 13 Oct 2011 16:51:48 -0400 (EDT)
David Miller <davem@xxxxxxxxxxxxx> wrote:

> >> 
> >> http://patchwork.ozlabs.org/patch/118693/
> >> http://patchwork.ozlabs.org/patch/118694/
> >> http://patchwork.ozlabs.org/patch/118695/
> >> http://patchwork.ozlabs.org/patch/118700/
> >> http://patchwork.ozlabs.org/patch/118696/
> >> http://patchwork.ozlabs.org/patch/118699/
> >> 
> >> This is a replacement for patch #1 in that series.
> >> 
> > 
> > Ok, let's add Andrew to the thread so this can go through -mm in 
> > preparation for that series.
> 
> It doesn't usually work like that, net-next is usually one of the first
> trees that Stephen pulls into -next, so this kind of simple dependency should
> go into my tree

yup.

> if the -mm developers give it an ACK and are OK with it.

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.

> +	__u32 page_offset;

I suggest this be called simply "offset".

> +	__u32 size;
> +#else
> +	__u16 page_offset;
> +	__u16 size;
> +#endif
> +};
> 	
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/