From: Benjamin Poirier <benjamin.poirier@gmail.com>
To: Coiby Xu <coiby.xu@gmail.com>
Cc: linux-staging@lists.linux.dev, netdev@vger.kernel.org
Subject: Re: About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO
Date: Wed, 5 May 2021 17:59:46 +0900 [thread overview]
Message-ID: <YJJegiK9mMvAEQwU@f3> (raw)
In-Reply-To: <20210504131421.mijffwcruql2fupn@Rk>
[-- Attachment #1: Type: text/plain, Size: 3878 bytes --]
On 2021-05-04 21:14 +0800, Coiby Xu wrote:
> Hi Benjamin,
>
> As you have known, I'm working on improving drivers/staging/qlge. I'm
> not sure if I correctly understand some TODO items. Since you wrote the TODO
> list, could you explain some of the items or comment on the
> corresponding fix for me?
>
> > * while in that area, using two 8k buffers to store one 9k frame is a poor
> > choice of buffer size.
>
> Currently, LARGE_BUFFER_MAX_SIZE is defined as 8192. How about we simply
> changing LARGE_BUFFER_MAX_SIZE to 4096? This is what
> drivers/net/ethernet/intel/e1000 does for jumbo frame right now.
I think that frags of 4096 would be better for allocations than the
current scheme. However, I don't know if simply changing that define is
the only thing to do.
BTW, e1000 was written long ago and not updated much, so it's not the
reference I would look at generally. Sadly I don't do much kernel
development anymore so I don't know which one to recommend either :/ If
I had to guess, I'd say ixgbe is a device of a similar vintage whose
driver has seen a lot better work.
>
> > * in the "chain of large buffers" case, the driver uses an skb allocated with
> > head room but only puts data in the frags.
>
> Do you suggest implementing the copybreak feature which exists for e1000 for
> this driver, i.e., allocing a sk_buff and coping the header buffer into it?
No. From the "chain of large buffers" quote, I think I was referring to:
\ qlge_refill_sb
skb = __netdev_alloc_skb(qdev->ndev, SMALL_BUFFER_SIZE, gfp);
\ qlge_build_rx_skb
[...]
/*
* The data is in a chain of large buffers
[...]
skb_fill_page_desc(skb, i,
lbq_desc->p.pg_chunk.page,
lbq_desc->p.pg_chunk.offset, size);
[...]
__pskb_pull_tail(skb, hlen);
So out of SMALL_BUFFER_SIZE, only hlen is used. Since SMALL_BUFFER_SIZE
is only 256, I'm not sure now if this really has any impact. In fact it
seems in line with ex. what ixgbe does (IXGBE_RX_HDR_SIZE).
However, in the same area, there is also
skb = netdev_alloc_skb(qdev->ndev, length);
[...]
skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
lbq_desc->p.pg_chunk.offset,
length);
Why is the skb allocated with "length" size? Something like
skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
would be better I think. The head only needs enough space for the
subsequent hlen pull.
BTW, it looks like commit f8c047be5401 ("staging: qlge: use qlge_*
prefix to avoid namespace clashes with other qlogic drivers") missed
some structures like struct rx_ring. Defines like SMALL_BUFFER_SIZE
should also have a prefix.
>
> > * fix weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
> > qlge_set_multicast_list()).
>
> This issue of weird line wrapping is supposed to be all over. But I can
> only find the ql_set_routing_reg() calls in qlge_set_multicast_list have
> this problem,
>
> if (qlge_set_routing_reg
> (qdev, RT_IDX_PROMISCUOUS_SLOT, RT_IDX_VALID, 1)) {
>
> I can't find other places where functions calls put square and arguments
> in the new line. Could you give more hints?
Here are other examples of what I would call weird line wrapping:
status = qlge_validate_flash(qdev,
sizeof(struct flash_params_8000) /
sizeof(u16),
"8000");
status = qlge_wait_reg_rdy(qdev,
XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
[...]
I put that item towards the end of the TODO list because I think the
misshapen formatting and the ridiculous overuse of () in expressions
serve a useful purpose. They clearly point to the code that hasn't yet
been rewritten; they make it easy to know what code to audit. Because of
that, I strongly think it would be better to tackle the TODO list
(roughly) in order.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-05-05 8:59 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-04 13:14 About improving the qlge Ethernet driver by following drivers/staging/qlge/TODO Coiby Xu
2021-05-05 8:59 ` Benjamin Poirier [this message]
2021-05-07 1:32 ` Coiby Xu
2021-05-07 12:16 ` Benjamin Poirier
2021-05-07 13:25 ` Coiby Xu
2021-05-08 23:27 ` Coiby Xu
2021-05-09 7:51 ` Benjamin Poirier
2021-05-09 23:54 ` Coiby Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YJJegiK9mMvAEQwU@f3 \
--to=benjamin.poirier@gmail.com \
--cc=coiby.xu@gmail.com \
--cc=linux-staging@lists.linux.dev \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).