Re: [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB

[Jacob Keller <jacob.e.keller@intel.com>]

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