On 08.07.2021 07:47, Juergen Gross wrote:
> On 17.05.21 17:33, Jan Beulich wrote:
>> On 17.05.2021 17:22, Juergen Gross wrote:
>>> On 17.05.21 17:12, Jan Beulich wrote:
>>>> On 17.05.2021 16:23, Juergen Gross wrote:
>>>>> On 17.05.21 16:11, Jan Beulich wrote:
>>>>>> On 13.05.2021 12:02, Juergen Gross wrote:
>>>>>>> @@ -1574,10 +1580,16 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
>>>>>>> spin_lock_irqsave(&rinfo->ring_lock, flags);
>>>>>>> again:
>>>>>>> rp = rinfo->ring.sring->rsp_prod;
>>>>>>> + if (RING_RESPONSE_PROD_OVERFLOW(&rinfo->ring, rp)) {
>>>>>>> + pr_alert("%s: illegal number of responses %u\n",
>>>>>>> + info->gd->disk_name, rp - rinfo->ring.rsp_cons);
>>>>>>> + goto err;
>>>>>>> + }
>>>>>>> rmb(); /* Ensure we see queued responses up to 'rp'. */
>>>>>>
>>>>>> I think you want to insert after the barrier.
>>>>>
>>>>> Why? The relevant variable which is checked is "rp". The result of the
>>>>> check is in no way depending on the responses themselves. And any change
>>>>> of rsp_cons is protected by ring_lock, so there is no possibility of
>>>>> reading an old value here.
>>>>
>>>> But this is a standard double read situation: You might check a value
>>>> and then (via a separate read) use a different one past the barrier.
>>>
>>> Yes and no.
>>>
>>> rsp_cons should never be written by the other side, and additionally
>>> it would be read multiple times anyway.
>>
>> But I'm talking about rsp_prod, as that's what rp gets loaded from.
>
> Oh, now I get your problem.
>
> But shouldn't that better be solved by using READ_ONCE() for reading rp
> instead?
Not sure - the rmb() is needed anyway aiui, and hence you could as well
move your code addition.
Jan
