| Message ID | 20210621134902.83587-2-coiby.xu@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Improve the qlge driver based on drivers/staging/qlge/TODO | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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
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
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);
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 <coiby.xu@gmail.com> --- drivers/staging/qlge/TODO | 2 -- drivers/staging/qlge/qlge_main.c | 10 +++++----- 2 files changed, 5 insertions(+), 7 deletions(-)