Re: RFC: softint-based if_input

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Thu, Feb 4, 2016 at 12:36 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> On Thu, Feb 4, 2016 at 3:52 AM, Taylor R Campbell
> <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>>    Date: Wed, 3 Feb 2016 14:55:31 +0900
>>    From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>>
>>    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