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

[Bernard Metzler]

-----"Peter Zijlstra" <peterz@xxxxxxxxxxxxx> wrote: -----

>To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>From: "Peter Zijlstra" <peterz@xxxxxxxxxxxxx>
>Date: 07/12/2019 03:19PM
>Cc: "Jason Gunthorpe" <jgg@xxxxxxxx>, "Arnd Bergmann"
><arnd@xxxxxxxx>, "Doug Ledford" <dledford@xxxxxxxxxx>,
>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:
>> @@ -1131,6 +1131,10 @@ int siw_poll_cq(struct ib_cq *base_cq, int
>num_cqe, struct ib_wc *wc)
>>   *   number of not reaped CQE's regardless of its notification
>>   *   type and current or new CQ notification settings.
>>   *
>> + * This function gets called only by kernel consumers.
>> + * Notification state must immediately become visible to all
>> + * associated kernel producers (QP's).
>
>No amount of memory barriers can achieve that goal.
>
>
Oh right, that is correct. And it is not needed. This statement
is to bold. We simply want it asap. Actually, per API semantics, there is
no ordering guarantee between creating a completion queue event and
concurrently arming/disarming the completion queue. Since it is
not possible.
I use the barrier to minimize the likelihood a CQ event is missed
since the CQ is not yet visible to be re-armed to all producers.
But it can happen. This is what this optional
IB_CQ_REPORT_MISSED_EVENTS right below the re-arming is for.

Would be OK if we just adapt the comment from 'must immediately become visible'
to 'shall become visible asap'. That would reflect the intention.

Thanks very much!
Bernard.

int siw_req_notify_cq(struct ib_cq *base_cq, enum ib_cq_notify_flags flags)
{
        struct siw_cq *cq = to_siw_cq(base_cq);

        siw_dbg_cq(cq, "flags: 0x%02x\n", flags);

        if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_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);
        else
                /*
                 * Enable CQ event for any signalled completion.
                 * and make it visible to all associated producers.
                 */
                smp_store_mb(cq->notify->flags, SIW_NOTIFY_ALL);

        if (flags & IB_CQ_REPORT_MISSED_EVENTS)
                return cq->cq_put - cq->cq_get;

        return 0;
}