From: Greg Kroah-Hartman <gregkh@linuxfoundation•org>
To: Benjamin Poirier <bpoirier@suse•com>
Cc: David Miller <davem@davemloft•net>,
Manish Chopra <manishc@marvell•com>,
GR-Linux-NIC-Dev@marvell•com, netdev@vger•kernel.org
Subject: Re: [PATCH net-next 01/16] qlge: Remove irq_cnt
Date: Mon, 15 Jul 2019 11:48:41 +0200 [thread overview]
Message-ID: <20190715094841.GC2955@kroah.com> (raw)
In-Reply-To: <20190715014016.GA27883@f1>
On Mon, Jul 15, 2019 at 10:40:16AM +0900, Benjamin Poirier wrote:
> On 2019/06/17 16:48, Benjamin Poirier wrote:
> > qlge uses an irq enable/disable refcounting scheme that is:
> > * poorly implemented
> > Uses a spin_lock to protect accesses to the irq_cnt atomic variable
> > * buggy
> > Breaks when there is not a 1:1 sequence of irq - napi_poll, such as
> > when using SO_BUSY_POLL.
> > * unnecessary
> > The purpose or irq_cnt is to reduce irq control writes when
> > multiple work items result from one irq: the irq is re-enabled
> > after all work is done.
> > Analysis of the irq handler shows that there is only one case where
> > there might be two workers scheduled at once, and those have
> > separate irq masking bits.
> >
> > Therefore, remove irq_cnt.
> >
> > Additionally, we get a performance improvement:
> > perf stat -e cycles -a -r5 super_netperf 100 -H 192.168.33.1 -t TCP_RR
> >
> > Before:
> > 628560
> > 628056
> > 622103
> > 622744
> > 627202
> > [...]
> > 268,803,947,669 cycles ( +- 0.09% )
> >
> > After:
> > 636300
> > 634106
> > 634984
> > 638555
> > 634188
> > [...]
> > 259,237,291,449 cycles ( +- 0.19% )
> >
> > Signed-off-by: Benjamin Poirier <bpoirier@suse•com>
>
> David, Greg,
>
> Before I send v2 of this patchset, how about moving this driver out to
> staging? The hardware has been declared EOL by the vendor but what's
> more relevant to the Linux kernel is that the quality of this driver is
> not on par with many other mainline drivers, IMO. Over in staging, the
> code might benefit from the attention of interested parties (drive-by
> fixers) or fade away into obscurity.
>
> Now that I check, the code has >500 basic checkpatch issues. While
> working on the rx processing code for the current patchset, I noticed
> the following more structural issues:
> * commit 7c734359d350 ("qlge: Size RX buffers based on MTU.",
> v2.6.33-rc1) introduced dead code in the receive routines, which
> should be rewritten anyways by the admission of the author himself
> (see the comment above ql_build_rx_skb())
> * truesize accounting is incorrect (ex: a 9000B frame has truesize 10280
> while containing two frags of order-1 allocations, ie. >16K)
> * in some cases the driver allocates skbs with head room but only puts
> data in the frags
> * there is an inordinate amount of disparate debugging code, most of
> which is of questionable value. In particular, qlge_dbg.c has hundreds
> of lines of code bitrotting away in ifdef land (doesn't compile since
> commit 18c49b91777c ("qlge: do vlan cleanup", v3.1-rc1), 8 years ago).
> * triggering an ethtool regdump will instead hexdump a 176k struct to
> dmesg depending on some module parameters
> * many structures are defined full of holes, as we found in this
> thread already; are used inefficiently (struct rx_ring is used for rx
> and tx completions, with some members relevant to one case only); are
> initialized redundantly (ex. memset 0 after alloc_etherdev). The
> driver also has a habit of using runtime checks where compile time
> checks are possible.
> * in terms of namespace, the driver uses either qlge_, ql_ (used by
> other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with
> clashes, ex: struct ob_mac_iocb_req)
>
> I'm guessing other parts of the driver have more issues of the same
> nature. The driver also has many smaller issues like deprecated apis,
> duplicate or useless comments, weird code structure (some "while" loops
> that could be rewritten with simple "for", ex. ql_wait_reg_rdy()) and
> weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
> qlge_set_multicast_list()).
>
> I'm aware of some precedents for code moving from mainline to staging:
> 1bb8155080c6 ncpfs: move net/ncpfs to drivers/staging/ncpfs (v4.16-rc1)
> 6c391ff758eb irda: move drivers/net/irda to drivers/staging/irda/drivers
> (v4.14-rc1)
>
> What do you think is the best course of action in this case?
Feel free to move it to staging if you want it removed from the tree in
a few releases if no one is willing to do the work to keep it alive. If
someone comes along later, it's a trivial revert to add it back.
So send a patch moving it, with all of the information you listed above
in a TODO file for the directory, and I'll be glad to take it.
thanks,
greg k-h
prev parent reply other threads:[~2019-07-15 9:48 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-17 7:48 [PATCH net-next 01/16] qlge: Remove irq_cnt Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 02/16] qlge: Remove page_chunk.last_flag Benjamin Poirier
2019-06-26 9:12 ` Manish Chopra
2019-06-17 7:48 ` [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size Benjamin Poirier
2019-06-26 9:24 ` [EXT] " Manish Chopra
2019-06-26 11:37 ` Benjamin Poirier
2019-06-26 15:42 ` Willem de Bruijn
2019-06-28 8:57 ` Benjamin Poirier
2019-06-28 14:56 ` Willem de Bruijn
2019-06-17 7:48 ` [PATCH net-next 04/16] qlge: Remove bq_desc.maplen Benjamin Poirier
2019-06-26 9:31 ` Manish Chopra
2019-06-17 7:48 ` [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size Benjamin Poirier
2019-06-26 9:36 ` [EXT] " Manish Chopra
2019-06-26 11:39 ` Benjamin Poirier
2019-06-26 15:35 ` Willem de Bruijn
2019-06-17 7:48 ` [PATCH net-next 06/16] qlge: Remove useless dma synchronization calls Benjamin Poirier
2019-06-17 9:44 ` [EXT] " Manish Chopra
2019-06-18 2:51 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 07/16] qlge: Deduplicate rx buffer queue management Benjamin Poirier
2019-06-27 10:02 ` [EXT] " Manish Chopra
2019-07-09 2:10 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 08/16] qlge: Fix dma_sync_single calls Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 09/16] qlge: Remove rx_ring.type Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 10/16] qlge: Factor out duplicated expression Benjamin Poirier
2019-06-23 17:59 ` David Miller
2019-06-23 18:00 ` David Miller
2019-06-24 7:52 ` Benjamin Poirier
2019-06-25 18:32 ` Manish Chopra
2019-06-28 8:52 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 11/16] qlge: Remove qlge_bq.len & size Benjamin Poirier
2019-06-27 10:47 ` Manish Chopra
2019-07-09 6:52 ` Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 12/16] qlge: Remove useless memset Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 13/16] qlge: Replace memset with assignment Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 14/16] qlge: Update buffer queue prod index despite oom Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 15/16] qlge: Refill rx buffers up to multiple of 16 Benjamin Poirier
2019-06-17 7:48 ` [PATCH net-next 16/16] qlge: Refill empty buffer queues from wq Benjamin Poirier
2019-06-27 14:18 ` [EXT] " Manish Chopra
2019-07-10 1:18 ` Benjamin Poirier
2019-06-17 16:49 ` [PATCH net-next 01/16] qlge: Remove irq_cnt Manish Chopra
2019-06-26 8:59 ` Manish Chopra
2019-06-26 11:36 ` Benjamin Poirier
2019-06-26 13:21 ` Manish Chopra
2019-07-05 8:33 ` Benjamin Poirier
2019-07-15 1:40 ` Benjamin Poirier
2019-07-15 9:48 ` Greg Kroah-Hartman [this message]
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=20190715094841.GC2955@kroah.com \
--to=gregkh@linuxfoundation$(echo .)org \
--cc=GR-Linux-NIC-Dev@marvell$(echo .)com \
--cc=bpoirier@suse$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=manishc@marvell$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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