From: Kevin Hao <hidden> Date: 2021-01-31 07:54:38
Hi,
v2:
- Inline page_frag_alloc() and {netdev,napi}_alloc_frag()
- Adopt Vlastimil's suggestion and add his Acked-by
In the current implementation of napi_alloc_frag(), it doesn't have any
align guarantee for the returned buffer address. We would have to use
some ugly workarounds to make sure that we can get a align buffer
address for some Ethernet drivers. This patch series tries to introduce
some helper functions to make sure that an align buffer is returned.
Then we can drop the ugly workarounds and avoid the unnecessary memory
waste.
Kevin Hao (4):
mm: page_frag: Introduce page_frag_alloc_align()
net: Introduce {netdev,napi}_alloc_frag_align()
net: octeontx2: Use napi_alloc_frag_align() to avoid the memory waste
net: dpaa2: Use napi_alloc_frag_align() to avoid the memory waste
.../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 3 +--
.../marvell/octeontx2/nic/otx2_common.c | 3 +--
include/linux/gfp.h | 12 +++++++--
include/linux/skbuff.h | 22 ++++++++++++++--
mm/page_alloc.c | 8 +++---
net/core/skbuff.c | 25 +++++++------------
6 files changed, 46 insertions(+), 27 deletions(-)
--
2.29.2
From: Kevin Hao <hidden> Date: 2021-01-31 07:54:55
In the current implementation of page_frag_alloc(), it doesn't have
any align guarantee for the returned buffer address. But for some
hardwares they do require the DMA buffer to be aligned correctly,
so we would have to use some workarounds like below if the buffers
allocated by the page_frag_alloc() are used by these hardwares for
DMA.
buf = page_frag_alloc(really_needed_size + align);
buf = PTR_ALIGN(buf, align);
These codes seems ugly and would waste a lot of memories if the buffers
are used in a network driver for the TX/RX. So introduce
page_frag_alloc_align() to make sure that an aligned buffer address is
returned.
Signed-off-by: Kevin Hao <redacted>
Acked-by: Vlastimil Babka <redacted>
---
v2:
- Inline page_frag_alloc()
- Adopt Vlastimil's suggestion and add his Acked-by
include/linux/gfp.h | 12 ++++++++++--
mm/page_alloc.c | 8 +++++---
2 files changed, 15 insertions(+), 5 deletions(-)
From: Kevin Hao <hidden> Date: 2021-01-31 07:59:18
The napi_alloc_frag_align() will guarantee that a correctly align
buffer address is returned. So use this function to simplify the buffer
alloc and avoid the unnecessary memory waste.
Signed-off-by: Kevin Hao <redacted>
---
v2: No change.
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: Kevin Hao <hidden> Date: 2021-01-31 07:59:18
In the current implementation of {netdev,napi}_alloc_frag(), it doesn't
have any align guarantee for the returned buffer address, But for some
hardwares they do require the DMA buffer to be aligned correctly,
so we would have to use some workarounds like below if the buffers
allocated by the {netdev,napi}_alloc_frag() are used by these hardwares
for DMA.
buf = napi_alloc_frag(really_needed_size + align);
buf = PTR_ALIGN(buf, align);
These codes seems ugly and would waste a lot of memories if the buffers
are used in a network driver for the TX/RX. We have added the align
support for the page_frag functions, so add the corresponding
{netdev,napi}_frag functions.
Signed-off-by: Kevin Hao <redacted>
---
v2: Inline {netdev,napi}_alloc_frag().
include/linux/skbuff.h | 22 ++++++++++++++++++++--
net/core/skbuff.c | 25 +++++++++----------------
2 files changed, 29 insertions(+), 18 deletions(-)
From: Kevin Hao <hidden> Date: 2021-01-31 07:59:47
The napi_alloc_frag_align() will guarantee that a correctly align
buffer address is returned. So use this function to simplify the buffer
alloc and avoid the unnecessary memory waste.
Signed-off-by: Kevin Hao <redacted>
---
v2: No change.
drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Tested-by: Subbaraya Sundeep <sbhatta@marvell.com>
Thanks,
Sundeep
On Sun, Jan 31, 2021 at 1:49 PM Kevin Hao [off-list ref] wrote:
quoted hunk
The napi_alloc_frag_align() will guarantee that a correctly align
buffer address is returned. So use this function to simplify the buffer
alloc and avoid the unnecessary memory waste.
Signed-off-by: Kevin Hao <redacted>
---
v2: No change.
drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
On Sun, Jan 31, 2021 at 03:44:23PM +0800, Kevin Hao wrote:
In the current implementation of page_frag_alloc(), it doesn't have
any align guarantee for the returned buffer address. But for some
hardwares they do require the DMA buffer to be aligned correctly,
so we would have to use some workarounds like below if the buffers
allocated by the page_frag_alloc() are used by these hardwares for
DMA.
buf = page_frag_alloc(really_needed_size + align);
buf = PTR_ALIGN(buf, align);
These codes seems ugly and would waste a lot of memories if the buffers
are used in a network driver for the TX/RX.
Isn't the memory wasted even with this change?
I am not familiar with the frag allocator so I might be missing
something, but from what I understood each page_frag_cache keeps only
the offset inside the current page being allocated, offset which you
ALIGN_DOWN() to match the alignment requirement. I don't see how that
memory between the non-aligned and aligned offset is going to be used
again before the entire page is freed.
quoted hunk
So introduce
page_frag_alloc_align() to make sure that an aligned buffer address is
returned.
Signed-off-by: Kevin Hao <redacted>
Acked-by: Vlastimil Babka <redacted>
---
v2:
- Inline page_frag_alloc()
- Adopt Vlastimil's suggestion and add his Acked-by
include/linux/gfp.h | 12 ++++++++++--
mm/page_alloc.c | 8 +++++---
2 files changed, 15 insertions(+), 5 deletions(-)
On Sun, Jan 31, 2021 at 03:44:23PM +0800, Kevin Hao wrote:
quoted
In the current implementation of page_frag_alloc(), it doesn't have
any align guarantee for the returned buffer address. But for some
hardwares they do require the DMA buffer to be aligned correctly,
so we would have to use some workarounds like below if the buffers
allocated by the page_frag_alloc() are used by these hardwares for
DMA.
buf = page_frag_alloc(really_needed_size + align);
buf = PTR_ALIGN(buf, align);
These codes seems ugly and would waste a lot of memories if the buffers
are used in a network driver for the TX/RX.
Isn't the memory wasted even with this change?
Yes, but less of it. Not always full amount of align, but up to it. Perhaps even
zero.
I am not familiar with the frag allocator so I might be missing
something, but from what I understood each page_frag_cache keeps only
the offset inside the current page being allocated, offset which you
ALIGN_DOWN() to match the alignment requirement. I don't see how that
memory between the non-aligned and aligned offset is going to be used
again before the entire page is freed.
True, thath's how page_frag is designed. The align amounts would be most likely
too small to be usable anyway.
On Tue, Feb 02, 2021 at 12:48:28PM +0100, Vlastimil Babka wrote:
On 2/2/21 12:36 PM, Ioana Ciornei wrote:
quoted
On Sun, Jan 31, 2021 at 03:44:23PM +0800, Kevin Hao wrote:
quoted
In the current implementation of page_frag_alloc(), it doesn't have
any align guarantee for the returned buffer address. But for some
hardwares they do require the DMA buffer to be aligned correctly,
so we would have to use some workarounds like below if the buffers
allocated by the page_frag_alloc() are used by these hardwares for
DMA.
buf = page_frag_alloc(really_needed_size + align);
buf = PTR_ALIGN(buf, align);
These codes seems ugly and would waste a lot of memories if the buffers
are used in a network driver for the TX/RX.
Isn't the memory wasted even with this change?
Yes, but less of it. Not always full amount of align, but up to it. Perhaps even
zero.
Indeed, the worst case is still there but we gain by not allocating the
full 'size + align' all the time.
Thanks.
quoted
I am not familiar with the frag allocator so I might be missing
something, but from what I understood each page_frag_cache keeps only
the offset inside the current page being allocated, offset which you
ALIGN_DOWN() to match the alignment requirement. I don't see how that
memory between the non-aligned and aligned offset is going to be used
again before the entire page is freed.
True, thath's how page_frag is designed. The align amounts would be most likely
too small to be usable anyway.
On Sun, Jan 31, 2021 at 03:44:26PM +0800, Kevin Hao wrote:
The napi_alloc_frag_align() will guarantee that a correctly align
buffer address is returned. So use this function to simplify the buffer
alloc and avoid the unnecessary memory waste.
Signed-off-by: Kevin Hao <redacted>
From: Alexander Duyck <hidden> Date: 2021-02-02 16:20:08
On Sat, Jan 30, 2021 at 11:54 PM Kevin Hao [off-list ref] wrote:
quoted hunk
In the current implementation of page_frag_alloc(), it doesn't have
any align guarantee for the returned buffer address. But for some
hardwares they do require the DMA buffer to be aligned correctly,
so we would have to use some workarounds like below if the buffers
allocated by the page_frag_alloc() are used by these hardwares for
DMA.
buf = page_frag_alloc(really_needed_size + align);
buf = PTR_ALIGN(buf, align);
These codes seems ugly and would waste a lot of memories if the buffers
are used in a network driver for the TX/RX. So introduce
page_frag_alloc_align() to make sure that an aligned buffer address is
returned.
Signed-off-by: Kevin Hao <redacted>
Acked-by: Vlastimil Babka <redacted>
---
v2:
- Inline page_frag_alloc()
- Adopt Vlastimil's suggestion and add his Acked-by
include/linux/gfp.h | 12 ++++++++++--
mm/page_alloc.c | 8 +++++---
2 files changed, 15 insertions(+), 5 deletions(-)
I would make "align" unsigned since really we are using it as a mask.
Actually passing it as a mask might be even better. More on that
below.
quoted hunk
{
unsigned int size = PAGE_SIZE;
struct page *page;
@@ -5190,11 +5190,13 @@ void *page_frag_alloc(struct page_frag_cache *nc, } nc->pagecnt_bias--;+ if (align)+ offset = ALIGN_DOWN(offset, align); nc->offset = offset; return nc->va + offset; }-EXPORT_SYMBOL(page_frag_alloc);+EXPORT_SYMBOL(page_frag_alloc_align); /* * Frees a page fragment allocated out of either a compound or order 0 page.
Rather than using the conditional branch it might be better to just do
"offset &= align_mask". Then you would be adding at most 1 instruction
which can likely occur in parallel with the other work that is going
on versus the conditional branch which requires a test, jump, and then
the 3 alignment instructions to do the subtraction, inversion, and
AND.
However it would ripple through the other patches as you would also
need to update you other patches to assume ~0 in the unaligned case,
however with your masked cases you could just use the negative
alignment value to generate your mask which would likely be taken care
of by the compiler.
From: Alexander Duyck <hidden> Date: 2021-02-02 16:29:38
On Sun, Jan 31, 2021 at 12:17 AM Kevin Hao [off-list ref] wrote:
quoted hunk
In the current implementation of {netdev,napi}_alloc_frag(), it doesn't
have any align guarantee for the returned buffer address, But for some
hardwares they do require the DMA buffer to be aligned correctly,
so we would have to use some workarounds like below if the buffers
allocated by the {netdev,napi}_alloc_frag() are used by these hardwares
for DMA.
buf = napi_alloc_frag(really_needed_size + align);
buf = PTR_ALIGN(buf, align);
These codes seems ugly and would waste a lot of memories if the buffers
are used in a network driver for the TX/RX. We have added the align
support for the page_frag functions, so add the corresponding
{netdev,napi}_frag functions.
Signed-off-by: Kevin Hao <redacted>
---
v2: Inline {netdev,napi}_alloc_frag().
include/linux/skbuff.h | 22 ++++++++++++++++++++--
net/core/skbuff.c | 25 +++++++++----------------
2 files changed, 29 insertions(+), 18 deletions(-)
So one thing we may want to do is actually split this up so that we
have a __netdev_alloc_frag_align function that is called by one of two
inline functions. The standard netdev_alloc_frag would be like what
you have here, however we would be passing ~0 for the mask.
The "align" version would be taking in an unsigned int align value and
converting it to a mask. The idea is that your mask value is likely a
constant so converting the constant to a mask would be much easier to
do in an inline function as the compiler can take care of converting
the value during compile time.
An added value to that is you could also add tests to the align value
to guarantee that the value being passed is a power of 2 so that it
works with the alignment mask generation as expected.
quoted hunk
struct sk_buff *__netdev_alloc_skb(struct net_device *dev, unsigned int length,
gfp_t gfp_mask);
@@ -2877,7 +2889,13 @@ static inline void skb_free_frag(void *addr) page_frag_free(addr); }-void *napi_alloc_frag(unsigned int fragsz);+void *napi_alloc_frag_align(unsigned int fragsz, int align);++static inline void *napi_alloc_frag(unsigned int fragsz)+{+ return napi_alloc_frag_align(fragsz, 0);+}+ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int length, gfp_t gfp_mask); static inline struct sk_buff *napi_alloc_skb(struct napi_struct *napi,
Same for the __napi_alloc_frag code. You could probably convert the
__napi_alloc_frag below into an __napi_alloc_frag_align that you pass
a mask to. Then you could convert the other two functions to either
pass ~0 or the align value and add align value validation.
From: Kevin Hao <hidden> Date: 2021-02-04 06:41:08
On Tue, Feb 02, 2021 at 08:19:54AM -0800, Alexander Duyck wrote:
On Sat, Jan 30, 2021 at 11:54 PM Kevin Hao [off-list ref] wrote:
quoted
In the current implementation of page_frag_alloc(), it doesn't have
any align guarantee for the returned buffer address. But for some
hardwares they do require the DMA buffer to be aligned correctly,
so we would have to use some workarounds like below if the buffers
allocated by the page_frag_alloc() are used by these hardwares for
DMA.
buf = page_frag_alloc(really_needed_size + align);
buf = PTR_ALIGN(buf, align);
These codes seems ugly and would waste a lot of memories if the buffers
are used in a network driver for the TX/RX. So introduce
page_frag_alloc_align() to make sure that an aligned buffer address is
returned.
Signed-off-by: Kevin Hao <redacted>
Acked-by: Vlastimil Babka <redacted>
---
v2:
- Inline page_frag_alloc()
- Adopt Vlastimil's suggestion and add his Acked-by
include/linux/gfp.h | 12 ++++++++++--
mm/page_alloc.c | 8 +++++---
2 files changed, 15 insertions(+), 5 deletions(-)
I would make "align" unsigned since really we are using it as a mask.
Actually passing it as a mask might be even better. More on that
below.
quoted
{
unsigned int size = PAGE_SIZE;
struct page *page;
@@ -5190,11 +5190,13 @@ void *page_frag_alloc(struct page_frag_cache *nc, } nc->pagecnt_bias--;+ if (align)+ offset = ALIGN_DOWN(offset, align); nc->offset = offset; return nc->va + offset; }-EXPORT_SYMBOL(page_frag_alloc);+EXPORT_SYMBOL(page_frag_alloc_align); /* * Frees a page fragment allocated out of either a compound or order 0 page.
Rather than using the conditional branch it might be better to just do
"offset &= align_mask". Then you would be adding at most 1 instruction
which can likely occur in parallel with the other work that is going
on versus the conditional branch which requires a test, jump, and then
the 3 alignment instructions to do the subtraction, inversion, and
AND.
On arm64:
if (align)
offset = ALIGN_DOWN(offset, align);
4b1503e2 neg w2, w21
710002bf cmp w21, #0x0
0a020082 and w2, w4, w2
1a841044 csel w4, w2, w4, ne // ne = any
offset &= align_mask
0a0402a4 and w4, w21, w4
Yes, we do cut 3 instructions by using align mask.
However it would ripple through the other patches as you would also
need to update you other patches to assume ~0 in the unaligned case,
however with your masked cases you could just use the negative
alignment value to generate your mask which would likely be taken care
of by the compiler.
From: Kevin Hao <hidden> Date: 2021-02-04 06:48:32
On Tue, Feb 02, 2021 at 08:26:19AM -0800, Alexander Duyck wrote:
On Sun, Jan 31, 2021 at 12:17 AM Kevin Hao [off-list ref] wrote:
quoted
In the current implementation of {netdev,napi}_alloc_frag(), it doesn't
have any align guarantee for the returned buffer address, But for some
hardwares they do require the DMA buffer to be aligned correctly,
so we would have to use some workarounds like below if the buffers
allocated by the {netdev,napi}_alloc_frag() are used by these hardwares
for DMA.
buf = napi_alloc_frag(really_needed_size + align);
buf = PTR_ALIGN(buf, align);
These codes seems ugly and would waste a lot of memories if the buffers
are used in a network driver for the TX/RX. We have added the align
support for the page_frag functions, so add the corresponding
{netdev,napi}_frag functions.
Signed-off-by: Kevin Hao <redacted>
---
v2: Inline {netdev,napi}_alloc_frag().
include/linux/skbuff.h | 22 ++++++++++++++++++++--
net/core/skbuff.c | 25 +++++++++----------------
2 files changed, 29 insertions(+), 18 deletions(-)
So one thing we may want to do is actually split this up so that we
have a __netdev_alloc_frag_align function that is called by one of two
inline functions. The standard netdev_alloc_frag would be like what
you have here, however we would be passing ~0 for the mask.
The "align" version would be taking in an unsigned int align value and
converting it to a mask. The idea is that your mask value is likely a
constant so converting the constant to a mask would be much easier to
do in an inline function as the compiler can take care of converting
the value during compile time.
An added value to that is you could also add tests to the align value
to guarantee that the value being passed is a power of 2 so that it
works with the alignment mask generation as expected.
Fair enough. Thanks Alexander.
Kevin
quoted
struct sk_buff *__netdev_alloc_skb(struct net_device *dev, unsigned int length,
gfp_t gfp_mask);
@@ -2877,7 +2889,13 @@ static inline void skb_free_frag(void *addr) page_frag_free(addr); }-void *napi_alloc_frag(unsigned int fragsz);+void *napi_alloc_frag_align(unsigned int fragsz, int align);++static inline void *napi_alloc_frag(unsigned int fragsz)+{+ return napi_alloc_frag_align(fragsz, 0);+}+ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int length, gfp_t gfp_mask); static inline struct sk_buff *napi_alloc_skb(struct napi_struct *napi,
Same for the __napi_alloc_frag code. You could probably convert the
__napi_alloc_frag below into an __napi_alloc_frag_align that you pass
a mask to. Then you could convert the other two functions to either
pass ~0 or the align value and add align value validation.