Re: [PATCH RFC 1/2] virtio-net: bql support

[Jason Wang <jasowang@redhat.com>]

On 2019/1/2 下午9:54, Michael S. Tsirkin wrote:
> On Wed, Jan 02, 2019 at 11:30:11AM +0800, Jason Wang wrote:
>> On 2018/12/31 上午2:48, Michael S. Tsirkin wrote:
>>> On Thu, Dec 27, 2018 at 06:04:53PM +0800, Jason Wang wrote:
>>>> On 2018/12/26 下午11:22, Michael S. Tsirkin wrote:
>>>>> On Thu, Dec 06, 2018 at 04:17:36PM +0800, Jason Wang wrote:
>>>>>> On 2018/12/6 上午6:54, Michael S. Tsirkin wrote:
>>>>>>> When use_napi is set, let's enable BQLs.  Note: some of the issues are
>>>>>>> similar to wifi.  It's worth considering whether something similar to
>>>>>>> commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be
>>>>>>> benefitial.
>>>>>> I've played a similar patch several days before. The tricky part is the mode
>>>>>> switching between napi and no napi. We should make sure when the packet is
>>>>>> sent and trakced by BQL,  it should be consumed by BQL as well.
>>>>> I just went over the patch again and I don't understand this comment.
>>>>> This patch only enabled BQL with tx napi.
>>>>>
>>>>> Thus there's no mode switching.
>>>>>
>>>>> What did I miss?
>>>> Consider the case:
>>>>
>>>>
>>>> TX NAPI is disabled:
>>>>
>>>> send N packets
>>>>
>>>> turn TX NAPI on:
>>>>
>>>> get tx interrupt
>>>>
>>>> BQL try to consume those packets when lead WARN for dql.
>>>>
>>>>
>>>> Thanks
>>> Can one really switch tx napi on and off? How?
>>> While root can change the napi_tx module parameter, I don't think
>>> that has any effect outside device probe time. What did I miss?
>>>
>>>
>>>
>> We support switch the mode through ethtool recently. See
>>
>> commit 0c465be183c7c57a26446df6ea96d8676b865f92
>> Author: Jason Wang <jasowang@redhat.com>
>> Date:   Tue Oct 9 10:06:26 2018 +0800
>>
>>      virtio_net: ethtool tx napi configuration
>>
>>      Implement ethtool .set_coalesce (-C) and .get_coalesce (-c) handlers.
>>      Interrupt moderation is currently not supported, so these accept and
>>      display the default settings of 0 usec and 1 frame.
>>
>>      Toggle tx napi through setting tx-frames. So as to not interfere
>>      with possible future interrupt moderation, value 1 means tx napi while
>>      value 0 means not.
>>
>>      Only allow the switching when device is down for simplicity.
>>
>>      Link: https://patchwork.ozlabs.org/patch/948149/
>>      Suggested-by: Jason Wang <jasowang@redhat.com>
>>      Signed-off-by: Willem de Bruijn <willemb@google.com>
>>      Signed-off-by: Jason Wang <jasowang@redhat.com>
>>      Signed-off-by: David S. Miller <davem@davemloft.net>
>>
>> Thanks
>
> It's disabled when device is up - isn't that enough?


Consider the case:

1) tx napi is disabled

2) send packets

3) tx napi is enabled through ethtool

4) get tx interrupt

5) BQL may start to consume packet that was sent when tx napi is 
disabled which will trigger BUG or WARN in dql

Thanks