Re: [PATCH v2 1/3] mm: Introduce page_size()

Re: [PATCH v2 1/3] mm: Introduce page_size()

[Matthew Wilcox]

On Mon, Jul 22, 2019 at 05:43:07PM -0700, Ira Weiny wrote:
> > diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
> > index 551bca6fef24..925be5942895 100644
> > --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> > +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> > @@ -1078,7 +1078,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> >  			bool merge;
> >  
> >  			if (page)
> > -				pg_size <<= compound_order(page);
> > +				pg_size = page_size(page);
> >  			if (off < pg_size &&
> >  			    skb_can_coalesce(skb, i, page, off)) {
> >  				merge = 1;
> > @@ -1105,8 +1105,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> >  							   __GFP_NORETRY,
> >  							   order);
> >  					if (page)
> > -						pg_size <<=
> > -							compound_order(page);
> > +						pg_size <<= order;
> 
> Looking at the code I see pg_size should be PAGE_SIZE right before this so why
> not just use the new call and remove the initial assignment?

This driver is really convoluted.  I wasn't certain I wouldn't break it
in some horrid way.  I made larger changes to it originally, then they
touched this part of the driver and I had to rework the patch to apply
on top of their changes.  So I did something more minimal.

This, on top of what's in Andrew's tree, would be my guess, but I don't
have the hardware.

diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
index 925be5942895..d4eb0fcd04c7 100644
--- a/drivers/crypto/chelsio/chtls/chtls_io.c
+++ b/drivers/crypto/chelsio/chtls/chtls_io.c
@@ -1073,7 +1073,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 		} else {
 			int i = skb_shinfo(skb)->nr_frags;
 			struct page *page = TCP_PAGE(sk);
-			int pg_size = PAGE_SIZE;
+			unsigned int pg_size = 0;
 			int off = TCP_OFF(sk);
 			bool merge;
 
@@ -1092,7 +1092,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 			if (page && off == pg_size) {
 				put_page(page);
 				TCP_PAGE(sk) = page = NULL;
-				pg_size = PAGE_SIZE;
+				pg_size = 0;
 			}
 
 			if (!page) {
@@ -1104,15 +1104,13 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 							   __GFP_NOWARN |
 							   __GFP_NORETRY,
 							   order);
-					if (page)
-						pg_size <<= order;
 				}
 				if (!page) {
 					page = alloc_page(gfp);
-					pg_size = PAGE_SIZE;
 				}
 				if (!page)
 					goto wait_for_memory;
+				pg_size = page_size(page);
 				off = 0;
 			}
 copy:

Re: [PATCH v2 1/3] mm: Introduce page_size()

[Ira Weiny]

On Tue, Jul 23, 2019 at 09:02:48AM -0700, Matthew Wilcox wrote:
> On Mon, Jul 22, 2019 at 05:43:07PM -0700, Ira Weiny wrote:
> > > diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > index 551bca6fef24..925be5942895 100644
> > > --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> > > +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > @@ -1078,7 +1078,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > >  			bool merge;
> > >  
> > >  			if (page)
> > > -				pg_size <<= compound_order(page);
> > > +				pg_size = page_size(page);
> > >  			if (off < pg_size &&
> > >  			    skb_can_coalesce(skb, i, page, off)) {
> > >  				merge = 1;
> > > @@ -1105,8 +1105,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > >  							   __GFP_NORETRY,
> > >  							   order);
> > >  					if (page)
> > > -						pg_size <<=
> > > -							compound_order(page);
> > > +						pg_size <<= order;
> > 
> > Looking at the code I see pg_size should be PAGE_SIZE right before this so why
> > not just use the new call and remove the initial assignment?
> 
> This driver is really convoluted.

Agreed...

>
> I wasn't certain I wouldn't break it
> in some horrid way.  I made larger changes to it originally, then they
> touched this part of the driver and I had to rework the patch to apply
> on top of their changes.  So I did something more minimal.
> 
> This, on top of what's in Andrew's tree, would be my guess, but I don't
> have the hardware.
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
> index 925be5942895..d4eb0fcd04c7 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -1073,7 +1073,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  		} else {
>  			int i = skb_shinfo(skb)->nr_frags;
>  			struct page *page = TCP_PAGE(sk);
> -			int pg_size = PAGE_SIZE;
> +			unsigned int pg_size = 0;
>  			int off = TCP_OFF(sk);
>  			bool merge;
>  
> @@ -1092,7 +1092,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  			if (page && off == pg_size) {
>  				put_page(page);
>  				TCP_PAGE(sk) = page = NULL;
> -				pg_size = PAGE_SIZE;
> +				pg_size = 0;

Yea...  I was not sure about this one at first...  :-/

>  			}
>  
>  			if (!page) {
> @@ -1104,15 +1104,13 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  							   __GFP_NOWARN |
>  							   __GFP_NORETRY,
>  							   order);
> -					if (page)
> -						pg_size <<= order;
>  				}
>  				if (!page) {
>  					page = alloc_page(gfp);
> -					pg_size = PAGE_SIZE;
>  				}
>  				if (!page)
>  					goto wait_for_memory;

Side note: why 2 checks for !page?

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> +				pg_size = page_size(page);
>  				off = 0;
>  			}
>  copy:

Re: [PATCH v2 1/3] mm: Introduce page_size()

[Matthew Wilcox]

On Tue, Jul 23, 2019 at 10:58:38AM -0700, Ira Weiny wrote:
> > @@ -1092,7 +1092,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> >  			if (page && off == pg_size) {
> >  				put_page(page);
> >  				TCP_PAGE(sk) = page = NULL;
> > -				pg_size = PAGE_SIZE;
> > +				pg_size = 0;
> 
> Yea...  I was not sure about this one at first...  :-/

I'm not sure we actually need to change pg_size here, but it seemed
appropriate to set it back to 0.

> >  							   __GFP_NORETRY,
> >  							   order);
> > -					if (page)
> > -						pg_size <<= order;
> >  				}
> >  				if (!page) {
> >  					page = alloc_page(gfp);
> > -					pg_size = PAGE_SIZE;
> >  				}
> >  				if (!page)
> >  					goto wait_for_memory;
> 
> Side note: why 2 checks for !page?

Because page is assigned to after the first check ...

Re: [PATCH v2 1/3] mm: Introduce page_size()

[Ira Weiny]

On Tue, Jul 23, 2019 at 11:14:13AM -0700, Matthew Wilcox wrote:
> On Tue, Jul 23, 2019 at 10:58:38AM -0700, Ira Weiny wrote:
> > > @@ -1092,7 +1092,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > >  			if (page && off == pg_size) {
> > >  				put_page(page);
> > >  				TCP_PAGE(sk) = page = NULL;
> > > -				pg_size = PAGE_SIZE;
> > > +				pg_size = 0;
> > 
> > Yea...  I was not sure about this one at first...  :-/
> 
> I'm not sure we actually need to change pg_size here, but it seemed
> appropriate to set it back to 0.
> 
> > >  							   __GFP_NORETRY,
> > >  							   order);
> > > -					if (page)
> > > -						pg_size <<= order;
> > >  				}
> > >  				if (!page) {
> > >  					page = alloc_page(gfp);
> > > -					pg_size = PAGE_SIZE;
> > >  				}
> > >  				if (!page)
> > >  					goto wait_for_memory;
> > 
> > Side note: why 2 checks for !page?
> 
> Because page is assigned to after the first check ...

Ah yea duh!  Sorry it is a bit hard to follow.

Ira

Re: [PATCH v2 1/3] mm: Introduce page_size()

[Matthew Wilcox]

On Tue, Jul 23, 2019 at 01:44:16PM -0700, Ira Weiny wrote:
> > > Side note: why 2 checks for !page?
> > 
> > Because page is assigned to after the first check ...
> 
> Ah yea duh!  Sorry it is a bit hard to follow.

This is one of those users who really wants the VM to fall back
automatically to any page order block it has on hand.  We talked about
it a bit in the MM track this year; not sure whether you were in the
room for it.

Re: [PATCH v2 1/3] mm: Introduce page_size()

[Andrew Morton]

On Tue, 23 Jul 2019 09:02:48 -0700 Matthew Wilcox <willy@infradead.org> wrote:

> On Mon, Jul 22, 2019 at 05:43:07PM -0700, Ira Weiny wrote:
> > > diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > index 551bca6fef24..925be5942895 100644
> > > --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> > > +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > @@ -1078,7 +1078,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > >  			bool merge;
> > >  
> > >  			if (page)
> > > -				pg_size <<= compound_order(page);
> > > +				pg_size = page_size(page);
> > >  			if (off < pg_size &&
> > >  			    skb_can_coalesce(skb, i, page, off)) {
> > >  				merge = 1;
> > > @@ -1105,8 +1105,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > >  							   __GFP_NORETRY,
> > >  							   order);
> > >  					if (page)
> > > -						pg_size <<=
> > > -							compound_order(page);
> > > +						pg_size <<= order;
> > 
> > Looking at the code I see pg_size should be PAGE_SIZE right before this so why
> > not just use the new call and remove the initial assignment?
> 
> This driver is really convoluted.  I wasn't certain I wouldn't break it
> in some horrid way.  I made larger changes to it originally, then they
> touched this part of the driver and I had to rework the patch to apply
> on top of their changes.  So I did something more minimal.
> 
> This, on top of what's in Andrew's tree, would be my guess, but I don't
> have the hardware.
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
> index 925be5942895..d4eb0fcd04c7 100644
> --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -1073,7 +1073,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  		} else {
>  			int i = skb_shinfo(skb)->nr_frags;
>  			struct page *page = TCP_PAGE(sk);
> -			int pg_size = PAGE_SIZE;
> +			unsigned int pg_size = 0;
>  			int off = TCP_OFF(sk);
>  			bool merge;
>  
> @@ -1092,7 +1092,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  			if (page && off == pg_size) {
>  				put_page(page);
>  				TCP_PAGE(sk) = page = NULL;
> -				pg_size = PAGE_SIZE;
> +				pg_size = 0;
>  			}
>  
>  			if (!page) {
> @@ -1104,15 +1104,13 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  							   __GFP_NOWARN |
>  							   __GFP_NORETRY,
>  							   order);
> -					if (page)
> -						pg_size <<= order;
>  				}
>  				if (!page) {
>  					page = alloc_page(gfp);
> -					pg_size = PAGE_SIZE;
>  				}
>  				if (!page)
>  					goto wait_for_memory;
> +				pg_size = page_size(page);
>  				off = 0;
>  			}

I didn't do anything with this.  I assume the original patch (which has
been in -next since July 22) is good and the above is merely a cleanup?

Re: [PATCH v2 1/3] mm: Introduce page_size()

[Matthew Wilcox]

On Fri, Sep 20, 2019 at 04:28:48PM -0700, Andrew Morton wrote:
> On Tue, 23 Jul 2019 09:02:48 -0700 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Mon, Jul 22, 2019 at 05:43:07PM -0700, Ira Weiny wrote:
> > > > diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > > index 551bca6fef24..925be5942895 100644
> > > > --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> > > > +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > > @@ -1078,7 +1078,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > > >  			bool merge;
> > > >  
> > > >  			if (page)
> > > > -				pg_size <<= compound_order(page);
> > > > +				pg_size = page_size(page);
> > > >  			if (off < pg_size &&
> > > >  			    skb_can_coalesce(skb, i, page, off)) {
> > > >  				merge = 1;
> > > > @@ -1105,8 +1105,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> > > >  							   __GFP_NORETRY,
> > > >  							   order);
> > > >  					if (page)
> > > > -						pg_size <<=
> > > > -							compound_order(page);
> > > > +						pg_size <<= order;
> > > 
> > > Looking at the code I see pg_size should be PAGE_SIZE right before this so why
> > > not just use the new call and remove the initial assignment?
> > 
> > This driver is really convoluted.  I wasn't certain I wouldn't break it
> > in some horrid way.  I made larger changes to it originally, then they
> > touched this part of the driver and I had to rework the patch to apply
> > on top of their changes.  So I did something more minimal.
> > 
> > This, on top of what's in Andrew's tree, would be my guess, but I don't
> > have the hardware.
> > 
> > diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
> > index 925be5942895..d4eb0fcd04c7 100644
> > --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> > +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> > @@ -1073,7 +1073,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> >  		} else {
> >  			int i = skb_shinfo(skb)->nr_frags;
> >  			struct page *page = TCP_PAGE(sk);
> > -			int pg_size = PAGE_SIZE;
> > +			unsigned int pg_size = 0;
> >  			int off = TCP_OFF(sk);
> >  			bool merge;
> >  
> > @@ -1092,7 +1092,7 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> >  			if (page && off == pg_size) {
> >  				put_page(page);
> >  				TCP_PAGE(sk) = page = NULL;
> > -				pg_size = PAGE_SIZE;
> > +				pg_size = 0;
> >  			}
> >  
> >  			if (!page) {
> > @@ -1104,15 +1104,13 @@ int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> >  							   __GFP_NOWARN |
> >  							   __GFP_NORETRY,
> >  							   order);
> > -					if (page)
> > -						pg_size <<= order;
> >  				}
> >  				if (!page) {
> >  					page = alloc_page(gfp);
> > -					pg_size = PAGE_SIZE;
> >  				}
> >  				if (!page)
> >  					goto wait_for_memory;
> > +				pg_size = page_size(page);
> >  				off = 0;
> >  			}
> 
> I didn't do anything with this.  I assume the original patch (which has
> been in -next since July 22) is good and the above is merely a cleanup?

Yes, just a cleanup.  Since Atul didn't offer an opinion, I assume
he doesn't care.

RE: [PATCH v2 1/3] mm: Introduce page_size()

[Weiny, Ira]

> On Fri, Sep 20, 2019 at 04:28:48PM -0700, Andrew Morton wrote:
> > On Tue, 23 Jul 2019 09:02:48 -0700 Matthew Wilcox <willy@infradead.org>
> wrote:
> >
> > > On Mon, Jul 22, 2019 at 05:43:07PM -0700, Ira Weiny wrote:
> > > > > diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c
> > > > > b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > > > index 551bca6fef24..925be5942895 100644
> > > > > --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> > > > > +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > > > @@ -1078,7 +1078,7 @@ int chtls_sendmsg(struct sock *sk, struct
> msghdr *msg, size_t size)
> > > > >  			bool merge;
> > > > >
> > > > >  			if (page)
> > > > > -				pg_size <<= compound_order(page);
> > > > > +				pg_size = page_size(page);
> > > > >  			if (off < pg_size &&
> > > > >  			    skb_can_coalesce(skb, i, page, off)) {
> > > > >  				merge = 1;
> > > > > @@ -1105,8 +1105,7 @@ int chtls_sendmsg(struct sock *sk, struct
> msghdr *msg, size_t size)
> > > > >
> __GFP_NORETRY,
> > > > >  							   order);
> > > > >  					if (page)
> > > > > -						pg_size <<=
> > > > > -
> 	compound_order(page);
> > > > > +						pg_size <<= order;
> > > >
> > > > Looking at the code I see pg_size should be PAGE_SIZE right before
> > > > this so why not just use the new call and remove the initial assignment?
> > >
> > > This driver is really convoluted.  I wasn't certain I wouldn't break
> > > it in some horrid way.  I made larger changes to it originally, then
> > > they touched this part of the driver and I had to rework the patch
> > > to apply on top of their changes.  So I did something more minimal.
> > >
> > > This, on top of what's in Andrew's tree, would be my guess, but I
> > > don't have the hardware.
> > >
> > > diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c
> > > b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > index 925be5942895..d4eb0fcd04c7 100644
> > > --- a/drivers/crypto/chelsio/chtls/chtls_io.c
> > > +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> > > @@ -1073,7 +1073,7 @@ int chtls_sendmsg(struct sock *sk, struct
> msghdr *msg, size_t size)
> > >  		} else {
> > >  			int i = skb_shinfo(skb)->nr_frags;
> > >  			struct page *page = TCP_PAGE(sk);
> > > -			int pg_size = PAGE_SIZE;
> > > +			unsigned int pg_size = 0;
> > >  			int off = TCP_OFF(sk);
> > >  			bool merge;
> > >
> > > @@ -1092,7 +1092,7 @@ int chtls_sendmsg(struct sock *sk, struct
> msghdr *msg, size_t size)
> > >  			if (page && off == pg_size) {
> > >  				put_page(page);
> > >  				TCP_PAGE(sk) = page = NULL;
> > > -				pg_size = PAGE_SIZE;
> > > +				pg_size = 0;
> > >  			}
> > >
> > >  			if (!page) {
> > > @@ -1104,15 +1104,13 @@ int chtls_sendmsg(struct sock *sk, struct
> msghdr *msg, size_t size)
> > >  							   __GFP_NOWARN |
> > >  							   __GFP_NORETRY,
> > >  							   order);
> > > -					if (page)
> > > -						pg_size <<= order;
> > >  				}
> > >  				if (!page) {
> > >  					page = alloc_page(gfp);
> > > -					pg_size = PAGE_SIZE;
> > >  				}
> > >  				if (!page)
> > >  					goto wait_for_memory;
> > > +				pg_size = page_size(page);
> > >  				off = 0;
> > >  			}
> >
> > I didn't do anything with this.  I assume the original patch (which
> > has been in -next since July 22) is good and the above is merely a cleanup?
> 
> Yes, just a cleanup.  Since Atul didn't offer an opinion, I assume he doesn't
> care.

Agreed I think what went in is fine.

Ira