Re: Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64

[Bernard Metzler]

-----"Jason Gunthorpe" <jgg@xxxxxxxx> wrote: -----

>To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>From: "Jason Gunthorpe" <jgg@xxxxxxxx>
>Date: 07/12/2019 03:53PM
>Cc: "Arnd Bergmann" <arnd@xxxxxxxx>, "Doug Ledford"
><dledford@xxxxxxxxxx>, "Peter Zijlstra" <peterz@xxxxxxxxxxxxx>,
>linux-rdma@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
>Subject: [EXTERNAL] Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on
>a u64
>
>On Fri, Jul 12, 2019 at 01:05:14PM +0000, Bernard Metzler wrote:
>> 
>> >To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>> >From: "Jason Gunthorpe" <jgg@xxxxxxxx>
>> >Date: 07/12/2019 02:03PM
>> >Cc: "Arnd Bergmann" <arnd@xxxxxxxx>, "Doug Ledford"
>> ><dledford@xxxxxxxxxx>, "Peter Zijlstra" <peterz@xxxxxxxxxxxxx>,
>> >linux-rdma@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
>> >Subject: [EXTERNAL] Re: [PATCH] rdma/siw: avoid smp_store_mb() on
>a
>> >u64
>> >
>> >On Fri, Jul 12, 2019 at 11:33:46AM +0000, Bernard Metzler wrote:
>> >> >diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
>> >> >b/drivers/infiniband/sw/siw/siw_verbs.c
>> >> >index 32dc79d0e898..41c5ab293fe1 100644
>> >> >+++ b/drivers/infiniband/sw/siw/siw_verbs.c
>> >> >@@ -1142,10 +1142,11 @@ int siw_req_notify_cq(struct ib_cq
>> >*base_cq,
>> >> >enum ib_cq_notify_flags flags)
>> >> > 
>> >> > 	if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED)
>> >> > 		/* CQ event for next solicited completion */
>> >> >-		smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED);
>> >> >+		WRITE_ONCE(*cq->notify, SIW_NOTIFY_SOLICITED);
>> >> > 	else
>> >> > 		/* CQ event for any signalled completion */
>> >> >-		smp_store_mb(*cq->notify, SIW_NOTIFY_ALL);
>> >> >+		WRITE_ONCE(*cq->notify, SIW_NOTIFY_ALL);
>> >> >+	smp_wmb();
>> >> > 
>> >> > 	if (flags & IB_CQ_REPORT_MISSED_EVENTS)
>> >> > 		return cq->cq_put - cq->cq_get;
>> >> 
>> >> 
>> >> Hi Arnd,
>> >> Many thanks for pointing that out! Indeed, this CQ notification
>> >> mechanism does not take 32 bit architectures into account.
>> >> Since we have only three flags to hold here, it's probably
>better
>> >> to make it a 32bit value. That would remove the issue w/o
>> >> introducing extra smp_wmb(). 
>> >
>> >I also prefer not to see smp_wmb() in drivers..
>> >
>> >> I'd prefer smp_store_mb(), since on some architectures it shall
>be
>> >> more efficient.  That would also make it sufficient to use
>> >> READ_ONCE.
>> >
>> >The READ_ONCE is confusing to me too, if you need store_release
>> >semantics then the reader also needs to pair with load_acquite -
>> >otherwise it doesn't work.
>> >
>> >Still, we need to do something rapidly to fix the i386 build,
>please
>> >revise right away..
>> >
>> >Jason
>> >
>> >
>> 
>> We share CQ (completion queue) notification flags between
>application
>> (which may be user land) and producer (kernel QP's (queue pairs)).
>> Those flags can be written by both application and QP's. The
>application
>> writes those flags to let the driver know if it shall inform about
>new
>> work completions. It can write those flags at any time.
>> Only a kernel producer reads those flags to decide if
>> the CQ notification handler shall be kicked, if a new CQ element
>gets
>> added to the CQ. When kicking the completion handler, the driver
>resets the
>> notification flag, which must get re-armed by the application.
>
>This looks wrong to me.. a userspace notification re-arm cannot be
>lost, so have a split READ/TEST/WRITE sequence can't possibly work?
>
>I'd expect an atomic test and clear here?

We cannot avoid the case that the application re-arms the
CQ only after a CQE got placed. That is why folks are polling the
CQ once after re-arming it - to make sure they do not miss the
very last and single CQE which would have produced a CQ event.

I do not think an atomic test and clear would change that picture.

Also, per RDMA verbs semantics, if the user would re-arm the CQ
more then once before it gets a CQ event, it will still get only one
CQ event if a new CQ element becomes ready. 
>
>
>> @@ -1141,11 +1145,17 @@ int siw_req_notify_cq(struct ib_cq
>*base_cq, enum ib_cq_notify_flags flags)
>>  	siw_dbg_cq(cq, "flags: 0x%02x\n", flags);
>>  
>>  	if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED)
>> -		/* CQ event for next solicited completion */
>> -		smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED);
>> +		/*
>> +		 * Enable CQ event for next solicited completion.
>> +		 * and make it visible to all associated producers.
>> +		 */
>> +		smp_store_mb(cq->notify->flags, SIW_NOTIFY_SOLICITED);
>
>But what is the 2nd piece of data to motivate the smp_store_mb?

Another core (such as a concurrent RX operation) shall see this
CQ being re-armed asap.

Best,
Bernard.
>
>Jason
>
>