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.