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:
signature.asc
Description: PGP signature
