Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
In-Reply-To: <20190712120328.GB27512@ziepe.ca>
From:   "Bernard Metzler" <BMT@zurich.ibm.com>
To:     "Jason Gunthorpe" <jgg@ziepe.ca>
Cc:     "Arnd Bergmann" <arnd@arndb.de>,
        "Doug Ledford" <dledford@redhat.com>,
        "Peter Zijlstra" <peterz@infradead.org>,
        linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Fri, 12 Jul 2019 12:27:26 +0000
MIME-Version: 1.0
Sensitivity: 
Importance: Normal
References: <20190712120328.GB27512@ziepe.ca>,<20190712085212.3901785-1-arnd@arndb.de>
 <OF05C1A780.433E36D1-ON00258435.003381DA-00258435.003F847E@notes.na.collabserv.com>
Content-Transfer-Encoding: 8BIT
Content-Type: text/plain; charset=UTF-8
Message-Id: <OF6D0AC131.1EAB95BA-ON00258435.00445D02-00258435.00446E58@notes.na.collabserv.com>
Subject: Re:  Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: -----

>To: "Bernard Metzler" <BMT@zurich.ibm.com>
>From: "Jason Gunthorpe" <jgg@ziepe.ca>
>Date: 07/12/2019 02:03PM
>Cc: "Arnd Bergmann" <arnd@arndb.de>, "Doug Ledford"
><dledford@redhat.com>, "Peter Zijlstra" <peterz@infradead.org>,
>linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
>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.
>

I used READ_ONCE since it is a potentially user land shared
object. Since we are under spinlock, we might just do a matching
WRITE_ONCE instead of the smp_store_mb() ??

Many thanks!
Bernard.

>Still, we need to do something rapidly to fix the i386 build, please
>revise right away..
>
>Jason
>
>