* Re: [PATCH v2 1/3] mm: Introduce page_size() [not found] ` <20190723004307.GB10284@iweiny-DESK2.sc.intel.com> @ 2019-07-23 16:02 ` Matthew Wilcox 2019-07-23 17:58 ` Ira Weiny 2019-09-20 23:28 ` Andrew Morton 0 siblings, 2 replies; 8+ messages in thread From: Matthew Wilcox @ 2019-07-23 16:02 UTC (permalink / raw) To: Ira Weiny; +Cc: Andrew Morton, linux-mm, Atul Gupta, linux-crypto 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: ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] mm: Introduce page_size() 2019-07-23 16:02 ` [PATCH v2 1/3] mm: Introduce page_size() Matthew Wilcox @ 2019-07-23 17:58 ` Ira Weiny 2019-07-23 18:14 ` Matthew Wilcox 2019-09-20 23:28 ` Andrew Morton 1 sibling, 1 reply; 8+ messages in thread From: Ira Weiny @ 2019-07-23 17:58 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Andrew Morton, linux-mm, Atul Gupta, linux-crypto 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: ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] mm: Introduce page_size() 2019-07-23 17:58 ` Ira Weiny @ 2019-07-23 18:14 ` Matthew Wilcox 2019-07-23 20:44 ` Ira Weiny 0 siblings, 1 reply; 8+ messages in thread From: Matthew Wilcox @ 2019-07-23 18:14 UTC (permalink / raw) To: Ira Weiny; +Cc: Andrew Morton, linux-mm, Atul Gupta, linux-crypto 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 ... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] mm: Introduce page_size() 2019-07-23 18:14 ` Matthew Wilcox @ 2019-07-23 20:44 ` Ira Weiny 2019-07-23 21:03 ` Matthew Wilcox 0 siblings, 1 reply; 8+ messages in thread From: Ira Weiny @ 2019-07-23 20:44 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Andrew Morton, linux-mm, Atul Gupta, linux-crypto 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] mm: Introduce page_size() 2019-07-23 20:44 ` Ira Weiny @ 2019-07-23 21:03 ` Matthew Wilcox 0 siblings, 0 replies; 8+ messages in thread From: Matthew Wilcox @ 2019-07-23 21:03 UTC (permalink / raw) To: Ira Weiny; +Cc: Andrew Morton, linux-mm, Atul Gupta, linux-crypto 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] mm: Introduce page_size() 2019-07-23 16:02 ` [PATCH v2 1/3] mm: Introduce page_size() Matthew Wilcox 2019-07-23 17:58 ` Ira Weiny @ 2019-09-20 23:28 ` Andrew Morton 2019-09-21 1:09 ` Matthew Wilcox 1 sibling, 1 reply; 8+ messages in thread From: Andrew Morton @ 2019-09-20 23:28 UTC (permalink / raw) To: Matthew Wilcox; +Cc: Ira Weiny, linux-mm, Atul Gupta, linux-crypto 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] mm: Introduce page_size() 2019-09-20 23:28 ` Andrew Morton @ 2019-09-21 1:09 ` Matthew Wilcox 2019-09-22 2:13 ` Weiny, Ira 0 siblings, 1 reply; 8+ messages in thread From: Matthew Wilcox @ 2019-09-21 1:09 UTC (permalink / raw) To: Andrew Morton; +Cc: Ira Weiny, linux-mm, Atul Gupta, linux-crypto 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 1/3] mm: Introduce page_size() 2019-09-21 1:09 ` Matthew Wilcox @ 2019-09-22 2:13 ` Weiny, Ira 0 siblings, 0 replies; 8+ messages in thread From: Weiny, Ira @ 2019-09-22 2:13 UTC (permalink / raw) To: Matthew Wilcox, Andrew Morton Cc: linux-mm@kvack.org, Atul Gupta, linux-crypto@vger.kernel.org > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-09-22 2:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20190721104612.19120-1-willy@infradead.org>
[not found] ` <20190721104612.19120-2-willy@infradead.org>
[not found] ` <20190723004307.GB10284@iweiny-DESK2.sc.intel.com>
2019-07-23 16:02 ` [PATCH v2 1/3] mm: Introduce page_size() Matthew Wilcox
2019-07-23 17:58 ` Ira Weiny
2019-07-23 18:14 ` Matthew Wilcox
2019-07-23 20:44 ` Ira Weiny
2019-07-23 21:03 ` Matthew Wilcox
2019-09-20 23:28 ` Andrew Morton
2019-09-21 1:09 ` Matthew Wilcox
2019-09-22 2:13 ` Weiny, Ira
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox