Message-ID: <20210507013239.4kmzsxtxnrpdqhuk@Rk>
Date:   Fri, 7 May 2021 09:32:39 +0800
From:   Coiby Xu <coiby.xu@...il.com>
To:     Benjamin Poirier <benjamin.poirier@...il.com>
Cc:     linux-staging@...ts.linux.dev, netdev@...r.kernel.org
Subject: Re: About improving the qlge Ethernet driver by following
 drivers/staging/qlge/TODO

On Wed, May 05, 2021 at 05:59:46PM +0900, Benjamin Poirier wrote:
>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.

Thanks! I think we can simply set it to 4096,
  - I did a basic test. There are two interfaces managed by this qlge
    driver. I started a HTTP server binded to one interface. And curl -I
    THE_OTHER_INTERFACE was fine.
  - ixgbe also set page order to 0 unless FCoE is 
     // drivers/net/ethernet/intel/ixgbe/ixgbe.h
     /*
      * FCoE requires that all Rx buffers be over 2200 bytes in length.  Since
      * this is twice the size of a half page we need to double the page order
      * for FCoE enabled Rx queues.
      */
     static inline unsigned int ixgbe_rx_bufsz(struct ixgbe_ring *ring)
     {
     	if (test_bit(__IXGBE_RX_3K_BUFFER, &ring->state))
     		return IXGBE_RXBUFFER_3K;
     #if (PAGE_SIZE < 8192)
     	if (ring_uses_build_skb(ring))
     		return IXGBE_MAX_2K_FRAME_BUILD_SKB;
     #endif
     	return IXGBE_RXBUFFER_2K;
     }
     
     static inline unsigned int ixgbe_rx_pg_order(struct ixgbe_ring *ring)
     {
     #if (PAGE_SIZE < 8192)
     	if (test_bit(__IXGBE_RX_3K_BUFFER, &ring->state))
     		return 1;
     #endif
     	return 0;
     }
   - e1000 does the same thing.
>
>>
>> > * 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).

Thanks for the clarification. It seems we needn't to change this place.

>
>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.

Thanks for the explanation! I think this place needs to modified. I'll
try to figure out how to reach this part of code so I can make sure the
change wouldn't introduce an issue.

Btw, qlge_process_mac_rx_page also has this issue,

     static void qlge_process_mac_rx_page(struct qlge_adapter *qdev,
     				     struct rx_ring *rx_ring,
     				     struct qlge_ib_mac_iocb_rsp *ib_mac_rsp,
     				     u32 length, u16 vlan_id)
     {
     	struct net_device *ndev = qdev->ndev;
     	struct sk_buff *skb = NULL;
     	void *addr;
     	struct qlge_bq_desc *lbq_desc = qlge_get_curr_lchunk(qdev, rx_ring);
     	struct napi_struct *napi = &rx_ring->napi;
     	size_t hlen = ETH_HLEN;
         ...
     	skb = netdev_alloc_skb(ndev, length);
     	skb_put_data(skb, addr, hlen);
         ...
     	skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
     			   lbq_desc->p.pg_chunk.offset + hlen, length - hlen);

The code path would include qlge_process_mac_rx_page by
$ ping -I enp94s0f0 -c 4 -s 8800 -M do 192.168.1.209
after enabling jumbo frame.  

>
>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.

Thanks for the reminding! When renaming rx_ring to a completion queue,
I'll add a prefix. I'll also add a prefix to other structures. 
>
>>
>> > * 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");

Oh, I also found this one but I think it more fits another TODO item,
i.e., "* fix weird indentation (all over, ex. the for loops in
qlge_get_stats())".

>
>	status = qlge_wait_reg_rdy(qdev,
>				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>
>[...]

Do you mean we should change it as follows,


	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
				               XGMAC_ADDR_XME);

"V=" in vim could detect some indentation problems but not the line
wrapping issue. So I just scanned the code manually to find this issue. 
Do you know there is a tool that could check if the code fits the kernel
coding style?

>
>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.

Thanks for this tip! I'll tackle the TODO list in order.

-- 
Best regards,
Coiby

Powered by blists - more mailing lists