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

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

[Ian Campbell]

A few network drivers currently use skb_frag_struct for this purpose but I have
patches which add additional fields and semantics there which these other uses
do not want.

A structure for reference sub-page regions seems like a generally useful thing
so do so instead of adding a network subsystem specific structure.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Jens Axboe <jaxboe@fusionio.com>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
[since v1: s/struct subpage/struct page_frag/ on advice from Christoph]
---
 include/linux/mm_types.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 774b895..575faaf 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -135,6 +135,17 @@ struct page {
 #endif
 ;
 
+struct page_frag {
+	struct page *page;
+#if (BITS_PER_LONG > 32) || (PAGE_SIZE >= 65536)
+	__u32 page_offset;
+	__u32 size;
+#else
+	__u16 page_offset;
+	__u16 size;
+#endif
+};
+
 typedef unsigned long __nocast vm_flags_t;
 
 /*
-- 
1.7.2.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Thu, 13 Oct 2011, Ian Campbell wrote:

> A few network drivers currently use skb_frag_struct for this purpose but I have
> patches which add additional fields and semantics there which these other uses
> do not want.
> 

Is this patch a part of a larger series that actually uses 
struct page_frag?  Probably a good idea to post them so we know it doesn't 
just lie there dormant.

> A structure for reference sub-page regions seems like a generally useful thing
> so do so instead of adding a network subsystem specific structure.
> 

Agreed.

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Jens Axboe <jaxboe@fusionio.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> [since v1: s/struct subpage/struct page_frag/ on advice from Christoph]

Acked-by: David Rientjes <rientjes@google.com>

> ---
>  include/linux/mm_types.h |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 774b895..575faaf 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -135,6 +135,17 @@ struct page {
>  #endif
>  ;
>  
> +struct page_frag {
> +	struct page *page;
> +#if (BITS_PER_LONG > 32) || (PAGE_SIZE >= 65536)
> +	__u32 page_offset;
> +	__u32 size;
> +#else
> +	__u16 page_offset;
> +	__u16 size;
> +#endif
> +};
> +
>  typedef unsigned long __nocast vm_flags_t;
>  
>  /*

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Miller]

From: David Rientjes <rientjes@google.com>
Date: Thu, 13 Oct 2011 13:33:45 -0700 (PDT)

> On Thu, 13 Oct 2011, Ian Campbell wrote:
> 
>> A few network drivers currently use skb_frag_struct for this purpose but I have
>> patches which add additional fields and semantics there which these other uses
>> do not want.
>> 
> 
> Is this patch a part of a larger series that actually uses 
> struct page_frag?  Probably a good idea to post them so we know it doesn't 
> just lie there dormant.

See:

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Thu, 13 Oct 2011, David Miller wrote:

> >> A few network drivers currently use skb_frag_struct for this purpose but I have
> >> patches which add additional fields and semantics there which these other uses
> >> do not want.
> >> 
> > 
> > Is this patch a part of a larger series that actually uses 
> > struct page_frag?  Probably a good idea to post them so we know it doesn't 
> > just lie there dormant.
> 
> See:
> 
> 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Miller]

From: David Rientjes <rientjes@google.com>
Date: Thu, 13 Oct 2011 13:49:35 -0700 (PDT)

> On Thu, 13 Oct 2011, David Miller wrote:
> 
>> >> A few network drivers currently use skb_frag_struct for this purpose but I have
>> >> patches which add additional fields and semantics there which these other uses
>> >> do not want.
>> >> 
>> > 
>> > Is this patch a part of a larger series that actually uses 
>> > struct page_frag?  Probably a good idea to post them so we know it doesn't 
>> > just lie there dormant.
>> 
>> See:
>> 
>> 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 if the -mm developers give it an ACK and are OK with it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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@davemloft.net> 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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jens Axboe]

On 2011-10-13 12:02, Ian Campbell wrote:
> A few network drivers currently use skb_frag_struct for this purpose but I have
> patches which add additional fields and semantics there which these other uses
> do not want.
> 
> A structure for reference sub-page regions seems like a generally useful thing
> so do so instead of adding a network subsystem specific structure.

Looks good to me, I can switch struct bio_vec over to this once it's in.

Acked-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Ian Campbell]

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.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Fri, 14 Oct 2011, Jens Axboe wrote:

> Looks good to me, I can switch struct bio_vec over to this once it's in.
> 

Looks like it could also be embedded within struct pipe_buffer.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jens Axboe]

On 2011-10-14 11:17, David Rientjes wrote:
> On Fri, 14 Oct 2011, Jens Axboe wrote:
> 
>> Looks good to me, I can switch struct bio_vec over to this once it's in.
>>
> 
> Looks like it could also be embedded within struct pipe_buffer.

Yep, that too.

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Ian Campbell]

On Fri, 2011-10-14 at 07:56 +0100, Ian Campbell wrote:
> 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?

I wasn't sure what to do here so I left it as it was, so the only change
here is s/page_offset/offset/. Switching to a u32 only version is
something we can easily do in the future if this scheme turns out to be
problematic.

Ian.

8<---------------------------------------------------------------

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

[David Rientjes]

On Tue, 18 Oct 2011, Ian Campbell wrote:

> From 806b74572ad63e2ed3ca69bb5640a55dc4475e73 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Mon, 3 Oct 2011 16:46:54 +0100
> Subject: [PATCH] mm: add a "struct page_frag" type containing a page, offset and length
> 
> A few network drivers currently use skb_frag_struct for this purpose but I have
> patches which add additional fields and semantics there which these other uses
> do not want.
> 
> A structure for reference sub-page regions seems like a generally useful thing
> so do so instead of adding a network subsystem specific structure.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Acked-by: Jens Axboe <jaxboe@fusionio.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: David Miller <davem@davemloft.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> [since v1: s/struct subpage/struct page_frag/ on advice from Christoph]
> [since v2: s/page_offset/offset/ on advice from Andrew]

Looks good, is this going to be going through net-next?

> ---
>  include/linux/mm_types.h |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 774b895..29971a5 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -135,6 +135,17 @@ struct page {
>  #endif
>  ;
>  
> +struct page_frag {
> +	struct page *page;
> +#if (BITS_PER_LONG > 32) || (PAGE_SIZE >= 65536)
> +	__u32 offset;
> +	__u32 size;
> +#else
> +	__u16 offset;
> +	__u16 size;
> +#endif
> +};
> +
>  typedef unsigned long __nocast vm_flags_t;
>  
>  /*

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrew Morton]

On Wed, 19 Oct 2011 22:36:30 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:

> On Tue, 18 Oct 2011, Ian Campbell wrote:
> 
> > From 806b74572ad63e2ed3ca69bb5640a55dc4475e73 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Mon, 3 Oct 2011 16:46:54 +0100
> > Subject: [PATCH] mm: add a "struct page_frag" type containing a page, offset and length
> > 
> > A few network drivers currently use skb_frag_struct for this purpose but I have
> > patches which add additional fields and semantics there which these other uses
> > do not want.
> > 
> > A structure for reference sub-page regions seems like a generally useful thing
> > so do so instead of adding a network subsystem specific structure.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Acked-by: Jens Axboe <jaxboe@fusionio.com>
> > Acked-by: David Rientjes <rientjes@google.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Cc: David Miller <davem@davemloft.net>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: linux-mm@kvack.org
> > Cc: linux-kernel@vger.kernel.org
> > [since v1: s/struct subpage/struct page_frag/ on advice from Christoph]
> > [since v2: s/page_offset/offset/ on advice from Andrew]
> 
> Looks good, is this going to be going through net-next?

yes please.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Miller]

From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Tue, 18 Oct 2011 09:49:38 +0100

> Subject: [PATCH] mm: add a "struct page_frag" type containing a page, offset and length
> 
> A few network drivers currently use skb_frag_struct for this purpose but I have
> patches which add additional fields and semantics there which these other uses
> do not want.
> 
> A structure for reference sub-page regions seems like a generally useful thing
> so do so instead of adding a network subsystem specific structure.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Acked-by: Jens Axboe <jaxboe@fusionio.com>
> Acked-by: David Rientjes <rientjes@google.com>

Applied, thanks Ian.

Please respin your skbfrag patches.

Thanks again.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Ian Campbell]

On Thu, 2011-10-20 at 09:59 +0100, David Miller wrote:
> From: Ian Campbell <Ian.Campbell@citrix.com>
> Date: Tue, 18 Oct 2011 09:49:38 +0100
> 
> > Subject: [PATCH] mm: add a "struct page_frag" type containing a page, offset and length
> > 
> > A few network drivers currently use skb_frag_struct for this purpose but I have
> > patches which add additional fields and semantics there which these other uses
> > do not want.
> > 
> > A structure for reference sub-page regions seems like a generally useful thing
> > so do so instead of adding a network subsystem specific structure.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Acked-by: Jens Axboe <jaxboe@fusionio.com>
> > Acked-by: David Rientjes <rientjes@google.com>
> 
> Applied, thanks Ian.
> 
> Please respin your skbfrag patches.

I think I must've hit send on the respin at about the same time you hit
send on your mail...

> 
> Thanks again.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>