Re: [PATCH v3 4/4] hw/nvme: add polling support

[Jinhao Fan]

at 10:11 PM, Klaus Jensen <its@irrelevant.dk> wrote:

> On Nov  8 12:39, John Levon wrote:
>> On Fri, Nov 04, 2022 at 07:32:12AM +0100, Klaus Jensen wrote:
>> 
>>> On Nov  3 21:19, Jinhao Fan wrote:
>>>> On 11/3/2022 8:10 PM, Klaus Jensen wrote:
>>>>> I agree that the spec is a little unclear on this point. In any case, in
>>>>> Linux, when the driver has decided that the sq tail must be updated,
>>>>> it will use this check:
>>>>> 
>>>>>   (new_idx - event_idx - 1) < (new_idx - old)
>>>> 
>>>> When eventidx is already behind, it's like:
>>>> 
>>>> 0
>>>> 1 <- event_idx
>>>> 2 <- old
>>>> 3 <- new_idx
>>>> 4
>>>> .
>>>> .
>>>> .
>>>> 
>>>> In this case, (new_idx - event_idx - 1) = 3-1-1 = 1 >= (new_idx - old) =
>>>> 3-2=1, so the host won't update sq tail. Where am I wrong in this example?
>>> 
>>> That becomes 1 >= 1, i.e. "true". So this will result in the driver
>>> doing an mmio doorbell write.
>> 
>> The code is:
>> 
>> static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx, u16 old) 
>>         
>> {                                                                            
>>     
>>        return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);         
>>    
>> }                                                                            
>>     
>> 
>> which per the above is "return 1 < 1;", or false. So the above case does 
>> *not*
>> do an mmio write. No?
> 
> Whelp.
> 
> Looks like I'm in the wrong here, apologies!

So disabling eventidx updates during polling has the potential of reducing
doorbell writes. But as Klaus observed removing this function does not cause
performance difference. So I guess only one command is processed during each
polling iteration.