Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
based on MTU") introduced page_chunk structure. We should add
qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/staging/qlge/TODO | 2 --
drivers/staging/qlge/qlge_main.c | 10 +++++-----
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index c76394b9451b..449d7dca478b 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -4,8 +4,6 @@
ql_build_rx_skb(). That function is now used exclusively to handle packets
that underwent header splitting but it still contains code to handle non
split cases.
-* truesize accounting is incorrect (ex: a 9000B frame has skb->truesize 10280
- while containing two frags of order-1 allocations, ie. >16K)
* while in that area, using two 8k buffers to store one 9k frame is a poor
choice of buffer size.
* in the "chain of large buffers" case, the driver uses an skb allocated with
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 19a02e958865..6dd69b689a58 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -1446,7 +1446,7 @@ static void qlge_process_mac_rx_gro_page(struct qlge_adapter *qdev,
skb->len += length;
skb->data_len += length;
- skb->truesize += length;
+ skb->truesize += qdev->lbq_buf_size;
skb_shinfo(skb)->nr_frags++;
rx_ring->rx_packets++;
@@ -1507,7 +1507,7 @@ static void qlge_process_mac_rx_page(struct qlge_adapter *qdev,
lbq_desc->p.pg_chunk.offset + hlen, length - hlen);
skb->len += length - hlen;
skb->data_len += length - hlen;
- skb->truesize += length - hlen;
+ skb->truesize += qdev->lbq_buf_size;
rx_ring->rx_packets++;
rx_ring->rx_bytes += skb->len;
@@ -1757,7 +1757,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
lbq_desc->p.pg_chunk.offset, length);
skb->len += length;
skb->data_len += length;
- skb->truesize += length;
+ skb->truesize += qdev->lbq_buf_size;
} else {
/*
* The headers and data are in a single large buffer. We
@@ -1783,7 +1783,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
length);
skb->len += length;
skb->data_len += length;
- skb->truesize += length;
+ skb->truesize += qdev->lbq_buf_size;
qlge_update_mac_hdr_len(qdev, ib_mac_rsp,
lbq_desc->p.pg_chunk.va,
&hlen);
@@ -1835,7 +1835,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
lbq_desc->p.pg_chunk.offset, size);
skb->len += size;
skb->data_len += size;
- skb->truesize += size;
+ skb->truesize += qdev->lbq_buf_size;
length -= size;
i++;
} while (length > 0);
--
2.32.0
On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
> Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
> based on MTU") introduced page_chunk structure. We should add
> qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
>
Add a Fixes tag.
The runtime impact of this is just that ethtool will report things
incorrectly, right? It's not 100% from the commit message. Could you
please edit the commit message so that an ignoramous like myself can
understand it?
Why is this an RFC instead of just a normal patch which we can apply?
regards,
dan carpenter
On Mon, Jun 21, 2021 at 05:10:27PM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
>> Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
>> based on MTU") introduced page_chunk structure. We should add
>> qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
>>
>
>Add a Fixes tag.
I will fix it in next version, thanks!
>
>The runtime impact of this is just that ethtool will report things
>incorrectly, right? It's not 100% from the commit message. Could you
>please edit the commit message so that an ignoramous like myself can
>understand it?
I'm not sure how it would affect ethtool. But according to "git log
--grep=truesize", it affects coalescing SKBs. Btw, I fixed the issue
according to the definition of truesize which according to Linux Kernel
Network by Rami Rosen, it's defined as follows,
> The total memory allocated for the SKB (including the SKB structure itself
> and the size of the allocated data block).
I'll edit the commit message to include it, thanks!
>
>Why is this an RFC instead of just a normal patch which we can apply?
After doing the tests mentioned in the cover letter, I found Red Hat's
network QE team has quite a rigorous test suite. But I needed to return
the machine before having the time to learn about the test suite and run
it by myself. So I mark it as an RFC before I borrow the machine again to
run the test suite.
>
>regards,
>dan carpenter
>
--
Best regards,
Coiby
On 2021-06-22 19:36 +0800, Coiby Xu wrote:
> On Mon, Jun 21, 2021 at 05:10:27PM +0300, Dan Carpenter wrote:
> > On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
> > > Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
> > > based on MTU") introduced page_chunk structure. We should add
> > > qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
> > >
> >
> > Add a Fixes tag.
>
> I will fix it in next version, thanks!
>
> >
> > The runtime impact of this is just that ethtool will report things
> > incorrectly, right? It's not 100% from the commit message. Could you
> > please edit the commit message so that an ignoramous like myself can
> > understand it?
truesize is used in socket memory accounting, the stuff behind sysctl
net.core.rmem_max, SO_RCVBUF, ss -m, ...
Some helpful chap wrote a page about it a while ago:
http://vger.kernel.org/~davem/skb_sk.html
>
> I'm not sure how it would affect ethtool. But according to "git log
> --grep=truesize", it affects coalescing SKBs. Btw, I fixed the issue
> according to the definition of truesize which according to Linux Kernel
> Network by Rami Rosen, it's defined as follows,
> > The total memory allocated for the SKB (including the SKB structure
> > itself and the size of the allocated data block).
>
> I'll edit the commit message to include it, thanks!
>
> >
> > Why is this an RFC instead of just a normal patch which we can apply?
>
> After doing the tests mentioned in the cover letter, I found Red Hat's
> network QE team has quite a rigorous test suite. But I needed to return the
> machine before having the time to learn about the test suite and run it by
> myself. So I mark it as an RFC before I borrow the machine again to run the
> test suite.
Interesting. Is this test suite based on a public project?
On Wed, Jun 23, 2021 at 01:55:15PM +0900, Benjamin Poirier wrote:
>On 2021-06-22 19:36 +0800, Coiby Xu wrote:
>> On Mon, Jun 21, 2021 at 05:10:27PM +0300, Dan Carpenter wrote:
>> > On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
>> > > Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
>> > > based on MTU") introduced page_chunk structure. We should add
>> > > qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
>> > >
>> >
>> > Add a Fixes tag.
>>
>> I will fix it in next version, thanks!
>>
>> >
>> > The runtime impact of this is just that ethtool will report things
>> > incorrectly, right? It's not 100% from the commit message. Could you
>> > please edit the commit message so that an ignoramous like myself can
>> > understand it?
>
>truesize is used in socket memory accounting, the stuff behind sysctl
>net.core.rmem_max, SO_RCVBUF, ss -m, ...
>
>Some helpful chap wrote a page about it a while ago:
>http://vger.kernel.org/~davem/skb_sk.html
Thanks for the explanation and the reference!
>
>>
>> I'm not sure how it would affect ethtool. But according to "git log
>> --grep=truesize", it affects coalescing SKBs. Btw, I fixed the issue
>> according to the definition of truesize which according to Linux Kernel
>> Network by Rami Rosen, it's defined as follows,
>> > The total memory allocated for the SKB (including the SKB structure
>> > itself and the size of the allocated data block).
>>
>> I'll edit the commit message to include it, thanks!
>>
>> >
>> > Why is this an RFC instead of just a normal patch which we can apply?
>>
>> After doing the tests mentioned in the cover letter, I found Red Hat's
>> network QE team has quite a rigorous test suite. But I needed to return the
>> machine before having the time to learn about the test suite and run it by
>> myself. So I mark it as an RFC before I borrow the machine again to run the
>> test suite.
>
>Interesting. Is this test suite based on a public project?
The test suite is written for Beaker [1] but it seems it's not public.
[1] https://fedoraproject.org/wiki/QA/Beaker
--
Best regards,
Coiby
On Thu, Jun 24, 2021 at 07:47:05PM +0800, Coiby Xu wrote:
>On Wed, Jun 23, 2021 at 01:55:15PM +0900, Benjamin Poirier wrote:
[..]
>>>> Why is this an RFC instead of just a normal patch which we can apply?
>>>
>>>After doing the tests mentioned in the cover letter, I found Red Hat's
>>>network QE team has quite a rigorous test suite. But I needed to return the
>>>machine before having the time to learn about the test suite and run it by
>>>myself. So I mark it as an RFC before I borrow the machine again to run the
>>>test suite.
>>
>>Interesting. Is this test suite based on a public project?
>
>The test suite is written for Beaker [1] but it seems it's not public.
>
>[1] https://fedoraproject.org/wiki/QA/Beaker
FYI, the tier-1 tests are part of the CKI project and are public [2].
[2] https://gitlab.com/cki-project/kernel-tests/-/tree/main/networking
--
Best regards,
Coiby