* Re: [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-11 22:22 [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB David Christensen
@ 2023-09-12 0:14 ` Jacob Keller
2023-09-12 20:48 ` David Christensen
2023-09-12 0:24 ` Nelson, Shannon
2023-09-14 22:02 ` [PATCH net v2] " David Christensen
2 siblings, 1 reply; 11+ messages in thread
From: Jacob Keller @ 2023-09-12 0:14 UTC (permalink / raw)
To: David Christensen, shannon.nelson, brett.creeley, drivers; +Cc: netdev
On 9/11/2023 3:22 PM, David Christensen wrote:
> The function ionic_rx_fill() uses 16bit math when calculating the
> the number of pages required for an RX descriptor given an interface
> MTU setting. If the system PAGE_SIZE >= 64KB, the frag_len and
> remain_len values will always be 0, causing unnecessary scatter-
> gather elements to be assigned to the RX descriptor, up to the
> maximum number of scatter-gather elements per descriptor.
>
> A similar change in ionic_rx_frags() is implemented for symmetry,
> but has not been observed as an issue since scatter-gather
> elements are not necessary for such larger page sizes.
>
> Fixes: 4b0a7539a372 ("ionic: implement Rx page reuse")
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> ---
Given this is a bug fix, it should probably have a subject of [PATCH
net] or [net] to indicate its targeting the net tree.
I'm not sure I follow the logic for frag_len and remain_len always being
zero, since typecasting unsigned values truncates the higher bytes
(technically its guaranteed by the standard to result in the smallest
value congruent modulo 2^16 for a 16bit typecast), so if page_offset was
non-zero then the resulting with the typecast should be as well.. but
either way its definitely not going to work as desired.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 26798fc635db..56502bc80e01 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -182,8 +182,8 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
> struct device *dev = q->dev;
> struct sk_buff *skb;
> unsigned int i;
> - u16 frag_len;
> - u16 len;
> + u32 frag_len;
> + u32 len;
>
> stats = q_to_rx_stats(q);
>
> @@ -207,7 +207,7 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
> return NULL;
> }
>
> - frag_len = min_t(u16, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> + frag_len = min_t(u32, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> len -= frag_len;
>
> dma_sync_single_for_cpu(dev,
> @@ -452,7 +452,7 @@ void ionic_rx_fill(struct ionic_queue *q)
>
> /* fill main descriptor - buf[0] */
> desc->addr = cpu_to_le64(buf_info->dma_addr + buf_info->page_offset);
> - frag_len = min_t(u16, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> + frag_len = min_t(u32, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> desc->len = cpu_to_le16(frag_len);
> remain_len -= frag_len;
> buf_info++;
> @@ -471,7 +471,7 @@ void ionic_rx_fill(struct ionic_queue *q)
> }
>
> sg_elem->addr = cpu_to_le64(buf_info->dma_addr + buf_info->page_offset);
> - frag_len = min_t(u16, remain_len, IONIC_PAGE_SIZE - buf_info->page_offset);
> + frag_len = min_t(u32, remain_len, IONIC_PAGE_SIZE - buf_info->page_offset);
> sg_elem->len = cpu_to_le16(frag_len);
> remain_len -= frag_len;
> buf_info++;
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-12 0:14 ` Jacob Keller
@ 2023-09-12 20:48 ` David Christensen
2023-09-12 21:13 ` Jacob Keller
0 siblings, 1 reply; 11+ messages in thread
From: David Christensen @ 2023-09-12 20:48 UTC (permalink / raw)
To: Jacob Keller, shannon.nelson, brett.creeley, drivers; +Cc: netdev
On 9/11/23 5:14 PM, Jacob Keller wrote:
>
>
> On 9/11/2023 3:22 PM, David Christensen wrote:
>> The function ionic_rx_fill() uses 16bit math when calculating the
>> the number of pages required for an RX descriptor given an interface
>> MTU setting. If the system PAGE_SIZE >= 64KB, the frag_len and
>> remain_len values will always be 0, causing unnecessary scatter-
>> gather elements to be assigned to the RX descriptor, up to the
>> maximum number of scatter-gather elements per descriptor.
>>
>> A similar change in ionic_rx_frags() is implemented for symmetry,
>> but has not been observed as an issue since scatter-gather
>> elements are not necessary for such larger page sizes.
>>
>> Fixes: 4b0a7539a372 ("ionic: implement Rx page reuse")
>> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
>> ---
>
> Given this is a bug fix, it should probably have a subject of [PATCH
> net] or [net] to indicate its targeting the net tree.
Will resend v2 with updated Subject line.
>
> I'm not sure I follow the logic for frag_len and remain_len always being
> zero, since typecasting unsigned values truncates the higher bytes
> (technically its guaranteed by the standard to result in the smallest
> value congruent modulo 2^16 for a 16bit typecast), so if page_offset was
> non-zero then the resulting with the typecast should be as well.. but
> either way its definitely not going to work as desired.
Sorry, tried condensing the explanation too much. I'm not sure how
frequently buf_info->page_offset is non-zero, but when
ionic_rx_page_alloc() allocates a new page, as happens during initial
driver load, it explicitly sets buf_info->page_offset to 0. As a
result, the remain_len value never decreases from the original frame
size (e.g. 1522) while frag_len is always calculated as 0 ((min_t(u16,
0x5F2, (0x1_0000 - 0) -> 0).
I'll update the the description in v2.
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-12 20:48 ` David Christensen
@ 2023-09-12 21:13 ` Jacob Keller
0 siblings, 0 replies; 11+ messages in thread
From: Jacob Keller @ 2023-09-12 21:13 UTC (permalink / raw)
To: David Christensen, shannon.nelson, brett.creeley, drivers; +Cc: netdev
On 9/12/2023 1:48 PM, David Christensen wrote:
>
>
> On 9/11/23 5:14 PM, Jacob Keller wrote:
>>
>>
>> On 9/11/2023 3:22 PM, David Christensen wrote:
>>> The function ionic_rx_fill() uses 16bit math when calculating the
>>> the number of pages required for an RX descriptor given an interface
>>> MTU setting. If the system PAGE_SIZE >= 64KB, the frag_len and
>>> remain_len values will always be 0, causing unnecessary scatter-
>>> gather elements to be assigned to the RX descriptor, up to the
>>> maximum number of scatter-gather elements per descriptor.
>>>
>>> A similar change in ionic_rx_frags() is implemented for symmetry,
>>> but has not been observed as an issue since scatter-gather
>>> elements are not necessary for such larger page sizes.
>>>
>>> Fixes: 4b0a7539a372 ("ionic: implement Rx page reuse")
>>> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
>>> ---
>>
>> Given this is a bug fix, it should probably have a subject of [PATCH
>> net] or [net] to indicate its targeting the net tree.
>
> Will resend v2 with updated Subject line.
>
>>
>> I'm not sure I follow the logic for frag_len and remain_len always being
>> zero, since typecasting unsigned values truncates the higher bytes
>> (technically its guaranteed by the standard to result in the smallest
>> value congruent modulo 2^16 for a 16bit typecast), so if page_offset was
>> non-zero then the resulting with the typecast should be as well.. but
>> either way its definitely not going to work as desired.
>
> Sorry, tried condensing the explanation too much. I'm not sure how
> frequently buf_info->page_offset is non-zero, but when
> ionic_rx_page_alloc() allocates a new page, as happens during initial
> driver load, it explicitly sets buf_info->page_offset to 0. As a
> result, the remain_len value never decreases from the original frame
> size (e.g. 1522) while frag_len is always calculated as 0 ((min_t(u16,
> 0x5F2, (0x1_0000 - 0) -> 0).
>
Yea that makes sense. When the page offset is zero this definitely
results in a zero value (given that PAGE_SIZE is always a power of 2, so
its congruent value is 0). In either case if PAGE_SIZE > 16bits this
will produce incorrect and unexpected results regardless.
> I'll update the the description in v2.
>
> Dave
Thanks!
-Jake
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-11 22:22 [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB David Christensen
2023-09-12 0:14 ` Jacob Keller
@ 2023-09-12 0:24 ` Nelson, Shannon
2023-09-12 22:31 ` David Christensen
2023-09-14 22:02 ` [PATCH net v2] " David Christensen
2 siblings, 1 reply; 11+ messages in thread
From: Nelson, Shannon @ 2023-09-12 0:24 UTC (permalink / raw)
To: David Christensen, brett.creeley, drivers; +Cc: netdev
On 9/11/2023 3:22 PM, David Christensen wrote:
>
> The function ionic_rx_fill() uses 16bit math when calculating the
> the number of pages required for an RX descriptor given an interface
> MTU setting. If the system PAGE_SIZE >= 64KB, the frag_len and
> remain_len values will always be 0, causing unnecessary scatter-
> gather elements to be assigned to the RX descriptor, up to the
> maximum number of scatter-gather elements per descriptor.
>
> A similar change in ionic_rx_frags() is implemented for symmetry,
> but has not been observed as an issue since scatter-gather
> elements are not necessary for such larger page sizes.
>
> Fixes: 4b0a7539a372 ("ionic: implement Rx page reuse")
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
The subject line prefix should have "net" in it to target the bug fix at
the right tree.
> ---
> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 26798fc635db..56502bc80e01 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -182,8 +182,8 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
> struct device *dev = q->dev;
> struct sk_buff *skb;
> unsigned int i;
> - u16 frag_len;
> - u16 len;
> + u32 frag_len;
> + u32 len;
>
> stats = q_to_rx_stats(q);
>
> @@ -207,7 +207,7 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
> return NULL;
> }
>
> - frag_len = min_t(u16, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> + frag_len = min_t(u32, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> len -= frag_len;
>
> dma_sync_single_for_cpu(dev,
> @@ -452,7 +452,7 @@ void ionic_rx_fill(struct ionic_queue *q)
>
> /* fill main descriptor - buf[0] */
> desc->addr = cpu_to_le64(buf_info->dma_addr + buf_info->page_offset);
> - frag_len = min_t(u16, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> + frag_len = min_t(u32, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> desc->len = cpu_to_le16(frag_len);
Hmm... using cpu_to_le16() on a 32-bit value looks suspect - it might
get forced to 16-bit, but looks funky, and might not be as successful in
a BigEndian environment.
Since the descriptor and sg_elem length fields are limited to 16-bit,
there might need to have something that assures that the resulting
lengths are never bigger than 64k - 1.
> remain_len -= frag_len;
> buf_info++;
> @@ -471,7 +471,7 @@ void ionic_rx_fill(struct ionic_queue *q)
> }
>
> sg_elem->addr = cpu_to_le64(buf_info->dma_addr + buf_info->page_offset);
> - frag_len = min_t(u16, remain_len, IONIC_PAGE_SIZE - buf_info->page_offset);
> + frag_len = min_t(u32, remain_len, IONIC_PAGE_SIZE - buf_info->page_offset);
> sg_elem->len = cpu_to_le16(frag_len);
ditto
> remain_len -= frag_len;
> buf_info++;
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-12 0:24 ` Nelson, Shannon
@ 2023-09-12 22:31 ` David Christensen
2023-09-14 20:28 ` Nelson, Shannon
0 siblings, 1 reply; 11+ messages in thread
From: David Christensen @ 2023-09-12 22:31 UTC (permalink / raw)
To: Nelson, Shannon, brett.creeley, drivers; +Cc: netdev
On 9/11/23 5:24 PM, Nelson, Shannon wrote:
>> @@ -452,7 +452,7 @@ void ionic_rx_fill(struct ionic_queue *q)
>>
>> /* fill main descriptor - buf[0] */
>> desc->addr = cpu_to_le64(buf_info->dma_addr +
>> buf_info->page_offset);
>> - frag_len = min_t(u16, len, IONIC_PAGE_SIZE -
>> buf_info->page_offset);
>> + frag_len = min_t(u32, len, IONIC_PAGE_SIZE -
>> buf_info->page_offset);
>> desc->len = cpu_to_le16(frag_len);
>
> Hmm... using cpu_to_le16() on a 32-bit value looks suspect - it might
> get forced to 16-bit, but looks funky, and might not be as successful in
> a BigEndian environment.
>
> Since the descriptor and sg_elem length fields are limited to 16-bit,
> there might need to have something that assures that the resulting
> lengths are never bigger than 64k - 1.
>
What do you think about this:
frag_len = min_t(u16, len, min_t(u32, 0xFFFF, IONIC_PAGE_SIZE -
buf_info->page_offset));
Can you think of a test case where buf_info->page_offset will be
non-zero that I can test locally?
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-12 22:31 ` David Christensen
@ 2023-09-14 20:28 ` Nelson, Shannon
2023-09-14 20:39 ` Nelson, Shannon
0 siblings, 1 reply; 11+ messages in thread
From: Nelson, Shannon @ 2023-09-14 20:28 UTC (permalink / raw)
To: David Christensen, brett.creeley, drivers; +Cc: netdev
On 9/12/2023 3:31 PM, David Christensen wrote:
>
> On 9/11/23 5:24 PM, Nelson, Shannon wrote:
>
>>> @@ -452,7 +452,7 @@ void ionic_rx_fill(struct ionic_queue *q)
>>>
>>> /* fill main descriptor - buf[0] */
>>> desc->addr = cpu_to_le64(buf_info->dma_addr +
>>> buf_info->page_offset);
>>> - frag_len = min_t(u16, len, IONIC_PAGE_SIZE -
>>> buf_info->page_offset);
>>> + frag_len = min_t(u32, len, IONIC_PAGE_SIZE -
>>> buf_info->page_offset);
>>> desc->len = cpu_to_le16(frag_len);
>>
>> Hmm... using cpu_to_le16() on a 32-bit value looks suspect - it might
>> get forced to 16-bit, but looks funky, and might not be as successful in
>> a BigEndian environment.
>>
>> Since the descriptor and sg_elem length fields are limited to 16-bit,
>> there might need to have something that assures that the resulting
>> lengths are never bigger than 64k - 1.
>>
>
> What do you think about this:
>
> frag_len = min_t(u16, len, min_t(u32, 0xFFFF, IONIC_PAGE_SIZE -
> buf_info->page_offset));
Yes, that looks a little safer.
We'll still need to do something about the 32-bit frag_len used in the
cpu_to_le16() call.
>
> Can you think of a test case where buf_info->page_offset will be
> non-zero that I can test locally?
No, I don't have one off-hand.
Thanks,
sln
>
> Dave
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-14 20:28 ` Nelson, Shannon
@ 2023-09-14 20:39 ` Nelson, Shannon
0 siblings, 0 replies; 11+ messages in thread
From: Nelson, Shannon @ 2023-09-14 20:39 UTC (permalink / raw)
To: David Christensen, brett.creeley, drivers; +Cc: netdev
On 9/14/2023 1:28 PM, 'Nelson, Shannon' via Pensando Drivers wrote:
>
> On 9/12/2023 3:31 PM, David Christensen wrote:
>>
>> On 9/11/23 5:24 PM, Nelson, Shannon wrote:
>>
>>>> @@ -452,7 +452,7 @@ void ionic_rx_fill(struct ionic_queue *q)
>>>>
>>>> /* fill main descriptor - buf[0] */
>>>> desc->addr = cpu_to_le64(buf_info->dma_addr +
>>>> buf_info->page_offset);
>>>> - frag_len = min_t(u16, len, IONIC_PAGE_SIZE -
>>>> buf_info->page_offset);
>>>> + frag_len = min_t(u32, len, IONIC_PAGE_SIZE -
>>>> buf_info->page_offset);
>>>> desc->len = cpu_to_le16(frag_len);
>>>
>>> Hmm... using cpu_to_le16() on a 32-bit value looks suspect - it might
>>> get forced to 16-bit, but looks funky, and might not be as successful in
>>> a BigEndian environment.
>>>
>>> Since the descriptor and sg_elem length fields are limited to 16-bit,
>>> there might need to have something that assures that the resulting
>>> lengths are never bigger than 64k - 1.
>>>
>>
>> What do you think about this:
>>
>> frag_len = min_t(u16, len, min_t(u32, 0xFFFF, IONIC_PAGE_SIZE -
>> buf_info->page_offset));
>
> Yes, that looks a little safer.
>
> We'll still need to do something about the 32-bit frag_len used in the
> cpu_to_le16() call.
>
>>
>> Can you think of a test case where buf_info->page_offset will be
>> non-zero that I can test locally?
>
> No, I don't have one off-hand.
Sorry, I was thinking about the case of a large page.
In the normal 1500 MTU case with heavy rx traffic (e.g. iperf receive,
pktgen, etc) we should see page split/offset use.
sln
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net v2] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-11 22:22 [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB David Christensen
2023-09-12 0:14 ` Jacob Keller
2023-09-12 0:24 ` Nelson, Shannon
@ 2023-09-14 22:02 ` David Christensen
2023-09-15 16:55 ` Nelson, Shannon
2023-09-16 10:50 ` patchwork-bot+netdevbpf
2 siblings, 2 replies; 11+ messages in thread
From: David Christensen @ 2023-09-14 22:02 UTC (permalink / raw)
To: shannon.nelson, brett.creeley, drivers; +Cc: netdev, David Christensen
From: David Christensen <drc@linux.vnet.ibm.com>
The ionic device supports a maximum buffer length of 16 bits (see
ionic_rxq_desc or ionic_rxq_sg_elem). When adding new buffers to
the receive rings, the function ionic_rx_fill() uses 16bit math when
calculating the number of pages to allocate for an RX descriptor,
given the interface's MTU setting. If the system PAGE_SIZE >= 64KB,
and the buf_info->page_offset is 0, the remain_len value will never
decrement from the original MTU value and the frag_len value will
always be 0, causing additional pages to be allocated as scatter-
gather elements unnecessarily.
A similar math issue exists in ionic_rx_frags(), but no failures
have been observed here since a 64KB page should not normally
require any scatter-gather elements at any legal Ethernet MTU size.
Fixes: 4b0a7539a372 ("ionic: implement Rx page reuse")
Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
---
drivers/net/ethernet/pensando/ionic/ionic_dev.h | 1 +
drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 10 +++++++---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
index 6aac98bcb9f4..aae4131f146a 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
@@ -187,6 +187,7 @@ typedef void (*ionic_desc_cb)(struct ionic_queue *q,
struct ionic_desc_info *desc_info,
struct ionic_cq_info *cq_info, void *cb_arg);
+#define IONIC_MAX_BUF_LEN ((u16)-1)
#define IONIC_PAGE_SIZE PAGE_SIZE
#define IONIC_PAGE_SPLIT_SZ (PAGE_SIZE / 2)
#define IONIC_PAGE_GFP_MASK (GFP_ATOMIC | __GFP_NOWARN |\
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index 26798fc635db..44466e8c5d77 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -207,7 +207,8 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
return NULL;
}
- frag_len = min_t(u16, len, IONIC_PAGE_SIZE - buf_info->page_offset);
+ frag_len = min_t(u16, len, min_t(u32, IONIC_MAX_BUF_LEN,
+ IONIC_PAGE_SIZE - buf_info->page_offset));
len -= frag_len;
dma_sync_single_for_cpu(dev,
@@ -452,7 +453,8 @@ void ionic_rx_fill(struct ionic_queue *q)
/* fill main descriptor - buf[0] */
desc->addr = cpu_to_le64(buf_info->dma_addr + buf_info->page_offset);
- frag_len = min_t(u16, len, IONIC_PAGE_SIZE - buf_info->page_offset);
+ frag_len = min_t(u16, len, min_t(u32, IONIC_MAX_BUF_LEN,
+ IONIC_PAGE_SIZE - buf_info->page_offset));
desc->len = cpu_to_le16(frag_len);
remain_len -= frag_len;
buf_info++;
@@ -471,7 +473,9 @@ void ionic_rx_fill(struct ionic_queue *q)
}
sg_elem->addr = cpu_to_le64(buf_info->dma_addr + buf_info->page_offset);
- frag_len = min_t(u16, remain_len, IONIC_PAGE_SIZE - buf_info->page_offset);
+ frag_len = min_t(u16, remain_len, min_t(u32, IONIC_MAX_BUF_LEN,
+ IONIC_PAGE_SIZE -
+ buf_info->page_offset));
sg_elem->len = cpu_to_le16(frag_len);
remain_len -= frag_len;
buf_info++;
--
2.39.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH net v2] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-14 22:02 ` [PATCH net v2] " David Christensen
@ 2023-09-15 16:55 ` Nelson, Shannon
2023-09-16 10:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 11+ messages in thread
From: Nelson, Shannon @ 2023-09-15 16:55 UTC (permalink / raw)
To: David Christensen, brett.creeley, drivers; +Cc: netdev, David Christensen
On 9/14/2023 3:02 PM, David Christensen wrote:
>
> From: David Christensen <drc@linux.vnet.ibm.com>
>
> The ionic device supports a maximum buffer length of 16 bits (see
> ionic_rxq_desc or ionic_rxq_sg_elem). When adding new buffers to
> the receive rings, the function ionic_rx_fill() uses 16bit math when
> calculating the number of pages to allocate for an RX descriptor,
> given the interface's MTU setting. If the system PAGE_SIZE >= 64KB,
> and the buf_info->page_offset is 0, the remain_len value will never
> decrement from the original MTU value and the frag_len value will
> always be 0, causing additional pages to be allocated as scatter-
> gather elements unnecessarily.
>
> A similar math issue exists in ionic_rx_frags(), but no failures
> have been observed here since a 64KB page should not normally
> require any scatter-gather elements at any legal Ethernet MTU size.
>
> Fixes: 4b0a7539a372 ("ionic: implement Rx page reuse")
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
Thanks
Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
> drivers/net/ethernet/pensando/ionic/ionic_dev.h | 1 +
> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 10 +++++++---
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> index 6aac98bcb9f4..aae4131f146a 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> @@ -187,6 +187,7 @@ typedef void (*ionic_desc_cb)(struct ionic_queue *q,
> struct ionic_desc_info *desc_info,
> struct ionic_cq_info *cq_info, void *cb_arg);
>
> +#define IONIC_MAX_BUF_LEN ((u16)-1)
> #define IONIC_PAGE_SIZE PAGE_SIZE
> #define IONIC_PAGE_SPLIT_SZ (PAGE_SIZE / 2)
> #define IONIC_PAGE_GFP_MASK (GFP_ATOMIC | __GFP_NOWARN |\
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 26798fc635db..44466e8c5d77 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -207,7 +207,8 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q,
> return NULL;
> }
>
> - frag_len = min_t(u16, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> + frag_len = min_t(u16, len, min_t(u32, IONIC_MAX_BUF_LEN,
> + IONIC_PAGE_SIZE - buf_info->page_offset));
> len -= frag_len;
>
> dma_sync_single_for_cpu(dev,
> @@ -452,7 +453,8 @@ void ionic_rx_fill(struct ionic_queue *q)
>
> /* fill main descriptor - buf[0] */
> desc->addr = cpu_to_le64(buf_info->dma_addr + buf_info->page_offset);
> - frag_len = min_t(u16, len, IONIC_PAGE_SIZE - buf_info->page_offset);
> + frag_len = min_t(u16, len, min_t(u32, IONIC_MAX_BUF_LEN,
> + IONIC_PAGE_SIZE - buf_info->page_offset));
> desc->len = cpu_to_le16(frag_len);
> remain_len -= frag_len;
> buf_info++;
> @@ -471,7 +473,9 @@ void ionic_rx_fill(struct ionic_queue *q)
> }
>
> sg_elem->addr = cpu_to_le64(buf_info->dma_addr + buf_info->page_offset);
> - frag_len = min_t(u16, remain_len, IONIC_PAGE_SIZE - buf_info->page_offset);
> + frag_len = min_t(u16, remain_len, min_t(u32, IONIC_MAX_BUF_LEN,
> + IONIC_PAGE_SIZE -
> + buf_info->page_offset));
> sg_elem->len = cpu_to_le16(frag_len);
> remain_len -= frag_len;
> buf_info++;
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH net v2] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
2023-09-14 22:02 ` [PATCH net v2] " David Christensen
2023-09-15 16:55 ` Nelson, Shannon
@ 2023-09-16 10:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-09-16 10:50 UTC (permalink / raw)
To: David Christensen; +Cc: shannon.nelson, brett.creeley, drivers, netdev, drc
Hello:
This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:
On Thu, 14 Sep 2023 18:02:52 -0400 you wrote:
> From: David Christensen <drc@linux.vnet.ibm.com>
>
> The ionic device supports a maximum buffer length of 16 bits (see
> ionic_rxq_desc or ionic_rxq_sg_elem). When adding new buffers to
> the receive rings, the function ionic_rx_fill() uses 16bit math when
> calculating the number of pages to allocate for an RX descriptor,
> given the interface's MTU setting. If the system PAGE_SIZE >= 64KB,
> and the buf_info->page_offset is 0, the remain_len value will never
> decrement from the original MTU value and the frag_len value will
> always be 0, causing additional pages to be allocated as scatter-
> gather elements unnecessarily.
>
> [...]
Here is the summary with links:
- [net,v2] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB
https://git.kernel.org/netdev/net/c/8f6b846b0a86
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread