* [net PATCH 0/2] Address recent issues found in netdev page_frag_alloc usage @ 2019-02-15 22:44 Alexander Duyck 2019-02-15 22:44 ` [net PATCH 1/2] mm: Use fixed constant in page_frag_alloc instead of size + 1 Alexander Duyck ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Alexander Duyck @ 2019-02-15 22:44 UTC (permalink / raw) To: netdev, davem; +Cc: linux-mm, linux-kernel, jannh This patch set addresses a couple of issues that I had pointed out to Jann Horn in response to a recent patch submission. The first issue is that I wanted to avoid the need to read/modify/write the size value in order to generate the value for pagecnt_bias. Instead we can just use a fixed constant which reduces the need for memory read operations and the overall number of instructions to update the pagecnt bias values. The other, and more important issue is, that apparently we were letting tun access the napi_alloc_cache indirectly through netdev_alloc_frag and as a result letting it create unaligned accesses via unaligned allocations. In order to prevent this I have added a call to SKB_DATA_ALIGN for the fragsz field so that we will keep the offset in the napi_alloc_cache SMP_CACHE_BYTES aligned. --- Alexander Duyck (2): mm: Use fixed constant in page_frag_alloc instead of size + 1 net: Do not allocate page fragments that are not skb aligned mm/page_alloc.c | 8 ++++---- net/core/skbuff.c | 4 ++++ 2 files changed, 8 insertions(+), 4 deletions(-) -- ^ permalink raw reply [flat|nested] 6+ messages in thread
* [net PATCH 1/2] mm: Use fixed constant in page_frag_alloc instead of size + 1 2019-02-15 22:44 [net PATCH 0/2] Address recent issues found in netdev page_frag_alloc usage Alexander Duyck @ 2019-02-15 22:44 ` Alexander Duyck 2023-02-17 9:30 ` Vlastimil Babka 2019-02-15 22:44 ` [net PATCH 2/2] net: Do not allocate page fragments that are not skb aligned Alexander Duyck 2019-02-17 23:50 ` [net PATCH 0/2] Address recent issues found in netdev page_frag_alloc usage David Miller 2 siblings, 1 reply; 6+ messages in thread From: Alexander Duyck @ 2019-02-15 22:44 UTC (permalink / raw) To: netdev, davem; +Cc: linux-mm, linux-kernel, jannh From: Alexander Duyck <alexander.h.duyck@linux.intel.com> This patch replaces the size + 1 value introduced with the recent fix for 1 byte allocs with a constant value. The idea here is to reduce code overhead as the previous logic would have to read size into a register, then increment it, and write it back to whatever field was being used. By using a constant we can avoid those memory reads and arithmetic operations in favor of just encoding the maximum value into the operation itself. Fixes: 2c2ade81741c ("mm: page_alloc: fix ref bias in page_frag_alloc() for 1-byte allocs") Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> --- mm/page_alloc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index ebb35e4d0d90..37ed14ad0b59 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -4857,11 +4857,11 @@ void *page_frag_alloc(struct page_frag_cache *nc, /* Even if we own the page, we do not use atomic_set(). * This would break get_page_unless_zero() users. */ - page_ref_add(page, size); + page_ref_add(page, PAGE_FRAG_CACHE_MAX_SIZE); /* reset page count bias and offset to start of new frag */ nc->pfmemalloc = page_is_pfmemalloc(page); - nc->pagecnt_bias = size + 1; + nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; nc->offset = size; } @@ -4877,10 +4877,10 @@ void *page_frag_alloc(struct page_frag_cache *nc, size = nc->size; #endif /* OK, page count is 0, we can safely set it */ - set_page_count(page, size + 1); + set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1); /* reset page count bias and offset to start of new frag */ - nc->pagecnt_bias = size + 1; + nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; offset = size - fragsz; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net PATCH 1/2] mm: Use fixed constant in page_frag_alloc instead of size + 1 2019-02-15 22:44 ` [net PATCH 1/2] mm: Use fixed constant in page_frag_alloc instead of size + 1 Alexander Duyck @ 2023-02-17 9:30 ` Vlastimil Babka 2023-03-20 15:14 ` Vlastimil Babka 0 siblings, 1 reply; 6+ messages in thread From: Vlastimil Babka @ 2023-02-17 9:30 UTC (permalink / raw) To: Alexander Duyck, netdev, davem; +Cc: linux-mm, linux-kernel, jannh On 2/15/19 23:44, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > This patch replaces the size + 1 value introduced with the recent fix for 1 > byte allocs with a constant value. > > The idea here is to reduce code overhead as the previous logic would have > to read size into a register, then increment it, and write it back to > whatever field was being used. By using a constant we can avoid those > memory reads and arithmetic operations in favor of just encoding the > maximum value into the operation itself. > > Fixes: 2c2ade81741c ("mm: page_alloc: fix ref bias in page_frag_alloc() for 1-byte allocs") > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > --- > mm/page_alloc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index ebb35e4d0d90..37ed14ad0b59 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -4857,11 +4857,11 @@ void *page_frag_alloc(struct page_frag_cache *nc, > /* Even if we own the page, we do not use atomic_set(). > * This would break get_page_unless_zero() users. > */ > - page_ref_add(page, size); > + page_ref_add(page, PAGE_FRAG_CACHE_MAX_SIZE); But this value can be theoretically too low when PAGE_SIZE > PAGE_FRAG_CACHE_MAX_SIZE? Such as on architectures with 64kB page size, while PAGE_FRAG_CACHE_MAX_SIZE is 32kB? Maybe impossible to exploit in practice thanks to the minimum alignment, but still IMHO we should be using the larger of PAGE_FRAG_CACHE_MAX_SIZE and PAGE_SIZE, which should still be a build-time constant, so not defeat the optimization. > > /* reset page count bias and offset to start of new frag */ > nc->pfmemalloc = page_is_pfmemalloc(page); > - nc->pagecnt_bias = size + 1; > + nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; > nc->offset = size; > } > > @@ -4877,10 +4877,10 @@ void *page_frag_alloc(struct page_frag_cache *nc, > size = nc->size; > #endif > /* OK, page count is 0, we can safely set it */ > - set_page_count(page, size + 1); > + set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1); > > /* reset page count bias and offset to start of new frag */ > - nc->pagecnt_bias = size + 1; > + nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1; > offset = size - fragsz; > } > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net PATCH 1/2] mm: Use fixed constant in page_frag_alloc instead of size + 1 2023-02-17 9:30 ` Vlastimil Babka @ 2023-03-20 15:14 ` Vlastimil Babka 0 siblings, 0 replies; 6+ messages in thread From: Vlastimil Babka @ 2023-03-20 15:14 UTC (permalink / raw) To: Alexander Duyck, netdev, davem; +Cc: linux-mm, linux-kernel, jannh On 2/17/23 10:30, Vlastimil Babka wrote: > On 2/15/19 23:44, Alexander Duyck wrote: >> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> >> >> This patch replaces the size + 1 value introduced with the recent fix for 1 >> byte allocs with a constant value. >> >> The idea here is to reduce code overhead as the previous logic would have >> to read size into a register, then increment it, and write it back to >> whatever field was being used. By using a constant we can avoid those >> memory reads and arithmetic operations in favor of just encoding the >> maximum value into the operation itself. >> >> Fixes: 2c2ade81741c ("mm: page_alloc: fix ref bias in page_frag_alloc() for 1-byte allocs") >> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> >> --- >> mm/page_alloc.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index ebb35e4d0d90..37ed14ad0b59 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -4857,11 +4857,11 @@ void *page_frag_alloc(struct page_frag_cache *nc, >> /* Even if we own the page, we do not use atomic_set(). >> * This would break get_page_unless_zero() users. >> */ >> - page_ref_add(page, size); >> + page_ref_add(page, PAGE_FRAG_CACHE_MAX_SIZE); > > But this value can be theoretically too low when PAGE_SIZE > > PAGE_FRAG_CACHE_MAX_SIZE? Such as on architectures with 64kB page size, > while PAGE_FRAG_CACHE_MAX_SIZE is 32kB? Nevermind, PAGE_FRAG_CACHE_MAX_SIZE would be 64kB because #define PAGE_FRAG_CACHE_MAX_SIZE __ALIGN_MASK(32768, ~PAGE_MASK) So all is fine, sorry for the noise. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [net PATCH 2/2] net: Do not allocate page fragments that are not skb aligned 2019-02-15 22:44 [net PATCH 0/2] Address recent issues found in netdev page_frag_alloc usage Alexander Duyck 2019-02-15 22:44 ` [net PATCH 1/2] mm: Use fixed constant in page_frag_alloc instead of size + 1 Alexander Duyck @ 2019-02-15 22:44 ` Alexander Duyck 2019-02-17 23:50 ` [net PATCH 0/2] Address recent issues found in netdev page_frag_alloc usage David Miller 2 siblings, 0 replies; 6+ messages in thread From: Alexander Duyck @ 2019-02-15 22:44 UTC (permalink / raw) To: netdev, davem; +Cc: linux-mm, linux-kernel, jannh From: Alexander Duyck <alexander.h.duyck@linux.intel.com> This patch addresses the fact that there are drivers, specifically tun, that will call into the network page fragment allocators with buffer sizes that are not cache aligned. Doing this could result in data alignment and DMA performance issues as these fragment pools are also shared with the skb allocator and any other devices that will use napi_alloc_frags or netdev_alloc_frags. Fixes: ffde7328a36d ("net: Split netdev_alloc_frag into __alloc_page_frag and add __napi_alloc_frag") Reported-by: Jann Horn <jannh@google.com> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> --- net/core/skbuff.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 26d848484912..2415d9cb9b89 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -356,6 +356,8 @@ static void *__netdev_alloc_frag(unsigned int fragsz, gfp_t gfp_mask) */ void *netdev_alloc_frag(unsigned int fragsz) { + fragsz = SKB_DATA_ALIGN(fragsz); + return __netdev_alloc_frag(fragsz, GFP_ATOMIC); } EXPORT_SYMBOL(netdev_alloc_frag); @@ -369,6 +371,8 @@ static void *__napi_alloc_frag(unsigned int fragsz, gfp_t gfp_mask) void *napi_alloc_frag(unsigned int fragsz) { + fragsz = SKB_DATA_ALIGN(fragsz); + return __napi_alloc_frag(fragsz, GFP_ATOMIC); } EXPORT_SYMBOL(napi_alloc_frag); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net PATCH 0/2] Address recent issues found in netdev page_frag_alloc usage 2019-02-15 22:44 [net PATCH 0/2] Address recent issues found in netdev page_frag_alloc usage Alexander Duyck 2019-02-15 22:44 ` [net PATCH 1/2] mm: Use fixed constant in page_frag_alloc instead of size + 1 Alexander Duyck 2019-02-15 22:44 ` [net PATCH 2/2] net: Do not allocate page fragments that are not skb aligned Alexander Duyck @ 2019-02-17 23:50 ` David Miller 2 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2019-02-17 23:50 UTC (permalink / raw) To: alexander.duyck; +Cc: netdev, linux-mm, linux-kernel, jannh From: Alexander Duyck <alexander.duyck@gmail.com> Date: Fri, 15 Feb 2019 14:44:05 -0800 > This patch set addresses a couple of issues that I had pointed out to Jann > Horn in response to a recent patch submission. > > The first issue is that I wanted to avoid the need to read/modify/write the > size value in order to generate the value for pagecnt_bias. Instead we can > just use a fixed constant which reduces the need for memory read operations > and the overall number of instructions to update the pagecnt bias values. > > The other, and more important issue is, that apparently we were letting tun > access the napi_alloc_cache indirectly through netdev_alloc_frag and as a > result letting it create unaligned accesses via unaligned allocations. In > order to prevent this I have added a call to SKB_DATA_ALIGN for the fragsz > field so that we will keep the offset in the napi_alloc_cache > SMP_CACHE_BYTES aligned. Series applied, thanks Alexander. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-20 15:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-15 22:44 [net PATCH 0/2] Address recent issues found in netdev page_frag_alloc usage Alexander Duyck 2019-02-15 22:44 ` [net PATCH 1/2] mm: Use fixed constant in page_frag_alloc instead of size + 1 Alexander Duyck 2023-02-17 9:30 ` Vlastimil Babka 2023-03-20 15:14 ` Vlastimil Babka 2019-02-15 22:44 ` [net PATCH 2/2] net: Do not allocate page fragments that are not skb aligned Alexander Duyck 2019-02-17 23:50 ` [net PATCH 0/2] Address recent issues found in netdev page_frag_alloc usage David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).