On Thu, Feb 4, 2016 at 12:36 PM, Ryota Ozaki <ozak...@netbsd.org> wrote: > On Thu, Feb 4, 2016 at 3:52 AM, Taylor R Campbell > <campbell+netbsd-tech-k...@mumble.net> wrote: >> Date: Wed, 3 Feb 2016 14:55:31 +0900 >> From: Ryota Ozaki <ozak...@netbsd.org> >> >> Here is a new patch: >> http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq2.diff >> The diff from v1 is here: >> http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq2-diff.diff >> >> Some comments: >> >> - if_percpuq_softint blocks interrupts while it processes the whole >> queue. It seems likely to me that this will limit throughput by >> preventing packets from coming in to the NIC at the same time as we're >> processing them in the kernel. > > I measured both cases, one big splnet vs. splnet per packet, > and there were no noticeable differences. In my measurements, the > former was slightly better than the latter. Under high load incoming > packets exceeds the number of packets that if_percpuq_softint can > process. In that case, packets are dropped at percpuq. One big splnet > helps to suppress the driver fetches and queues too much packets to > percpuq. > > That said, the difference is not big deal in favor of polling mode :) > >> >> - if_percpuq_dequeue must be called under splnet, unlike >> if_percpuq_enqueue. If you want this to be part of the contract, it >> should be documented or at least commented. But I think you probably >> want to push splnet into if_percpuq_dequeue, for symmetry with >> if_percpuq_enqueue and, more importantly, to let most of >> if_percpuq_softint run without splnet. > > Yeah, splnet in if_percpuq_dequeue is more symmetric. I adopt it. > >> >> - In a few cases, you've changed >> >> if_attach(ifp); >> somethingelse_attach(ifp); >> >> into >> >> if_initialize(ifp); >> somethingelse_attach(ifp); >> if_register(ifp); >> >> E.g., bcmeth does ether_ifattach(ifp) in the middle with your patch. >> Why? >> >> In answer to my own question: because you changed the intended API to >> split if_attach into if_initialize/if_register back in 2014, and >> deprecated if_attach, and I even participated in the thread about >> that, but evidently forgot. OK! >> >> https://mail-index.netbsd.org/tech-kern/2014/12/10/msg018242.html > > Anyway I have to write some documents/comments for driver developers. > Will do. > >> >> - You seem to have changed all the USB drivers to skip the ifq and go >> straight into the network stack. While this more or less makes sense >> as long as IPL_SOFTUSB = IPL_SOFTNET, as I noted before I expect that >> IPL_SOFTUSB will change fairly soon to be IPL_SOFTSERIAL instead, in >> which case you will need to go through the ifq and softnet softint. >> So I think you probably want to treat the USB drivers like all the >> other ones you're converting in bulk. > > Sure. The change may add some overhead though, it doesn't matter much. > I'll change so.
So here is latest patches that apply the above fixes: http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq3-diff.diff http://www.netbsd.org/~ozaki-r/softint-if_input-percpuq3.diff ozaki-r
