RE: [PATCH RFC] rds: ib: Reduce the contention caused by the asynchronous workers to flush the mr pool

[Praveen Kannoju]

> -----Original Message-----
> From: Leon Romanovsky [mailto:leon@xxxxxxxxxx]
> Sent: 20 January 2022 05:51 PM
> To: Praveen Kannoju <praveen.kannoju@xxxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxx>; Santosh Shilimkar
> <santosh.shilimkar@xxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>;
> kuba@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx;
> rds-devel@xxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Rama
> Nichanamatlu <rama.nichanamatlu@xxxxxxxxxx>; Rajesh
> Sivaramasubramaniom <rajesh.sivaramasubramaniom@xxxxxxxxxx>
> Subject: Re: [PATCH RFC] rds: ib: Reduce the contention caused by the
> asynchronous workers to flush the mr pool
> 
> On Thu, Jan 20, 2022 at 11:57:02AM +0000, Praveen Kannoju wrote:
> > > -----Original Message-----
> > > From: Leon Romanovsky [mailto:leon@xxxxxxxxxx]
> > > Sent: 20 January 2022 04:41 PM
> > > To: Praveen Kannoju <praveen.kannoju@xxxxxxxxxx>
> > > Cc: Jason Gunthorpe <jgg@xxxxxxxx>; Santosh Shilimkar
> > > <santosh.shilimkar@xxxxxxxxxx>; David S . Miller
> > > <davem@xxxxxxxxxxxxx>; kuba@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> > > linux-rdma@xxxxxxxxxxxxxxx; rds-devel@xxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; Rama Nichanamatlu
> > > <rama.nichanamatlu@xxxxxxxxxx>; Rajesh Sivaramasubramaniom
> > > <rajesh.sivaramasubramaniom@xxxxxxxxxx>
> > > Subject: Re: [PATCH RFC] rds: ib: Reduce the contention caused by
> > > the asynchronous workers to flush the mr pool
> > >
> > > On Thu, Jan 20, 2022 at 08:00:54AM +0000, Praveen Kannoju wrote:
> > > > -----Original Message-----
> > > > From: Leon Romanovsky [mailto:leon@xxxxxxxxxx]
> > > > Sent: 19 January 2022 08:26 PM
> > > > To: Praveen Kannoju <praveen.kannoju@xxxxxxxxxx>
> > > > Cc: Santosh Shilimkar <santosh.shilimkar@xxxxxxxxxx>; Jason
> > > > Gunthorpe <jgg@xxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>;
> > > > kuba@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> > > > linux-rdma@xxxxxxxxxxxxxxx; rds-devel@xxxxxxxxxxxxxx;
> > > > linux-kernel@xxxxxxxxxxxxxxx; Rama Nichanamatlu
> > > > <rama.nichanamatlu@xxxxxxxxxx>; Rajesh Sivaramasubramaniom
> > > > <rajesh.sivaramasubramaniom@xxxxxxxxxx>
> > > > Subject: Re: [PATCH RFC] rds: ib: Reduce the contention caused by
> > > > the asynchronous workers to flush the mr pool
> > > >
> > > > On Wed, Jan 19, 2022 at 11:46:16AM +0000, Praveen Kannoju wrote:
> > > > > -----Original Message-----
> > > > > From: Leon Romanovsky [mailto:leon@xxxxxxxxxx]
> > > > > Sent: 19 January 2022 12:29 PM
> > > > > To: Santosh Shilimkar <santosh.shilimkar@xxxxxxxxxx>
> > > > > Cc: Jason Gunthorpe <jgg@xxxxxxxx>; Praveen Kannoju
> > > > > <praveen.kannoju@xxxxxxxxxx>; David S . Miller
> > > > > <davem@xxxxxxxxxxxxx>; kuba@xxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx;
> > > > > linux-rdma@xxxxxxxxxxxxxxx; rds-devel@xxxxxxxxxxxxxx;
> > > > > linux-kernel@xxxxxxxxxxxxxxx; Rama Nichanamatlu
> > > > > <rama.nichanamatlu@xxxxxxxxxx>; Rajesh Sivaramasubramaniom
> > > > > <rajesh.sivaramasubramaniom@xxxxxxxxxx>
> > > > > Subject: Re: [PATCH RFC] rds: ib: Reduce the contention caused
> > > > > by the asynchronous workers to flush the mr pool
> > > > >
> > > > > On Tue, Jan 18, 2022 at 07:42:54PM +0000, Santosh Shilimkar wrote:
> > > > > > On Jan 18, 2022, at 11:17 AM, Jason Gunthorpe <jgg@xxxxxxxx>
> wrote:
> > > > > > >
> > > > > > > On Tue, Jan 18, 2022 at 04:48:43PM +0000, Santosh Shilimkar
> wrote:
> > > > > > >>
> > > > > > >>> On Jan 18, 2022, at 6:47 AM, Praveen Kannoju
> > > <praveen.kannoju@xxxxxxxxxx> wrote:
> > > > > > >>>
> > > > > > >>> This patch aims to reduce the number of asynchronous
> > > > > > >>> workers being spawned to execute the function
> "rds_ib_flush_mr_pool"
> > > > > > >>> during the high I/O situations. Synchronous call path's to
> > > > > > >>> this
> > > function "rds_ib_flush_mr_pool"
> > > > > > >>> will be executed without being disturbed. By reducing the
> > > > > > >>> number of processes contending to flush the mr pool, the
> > > > > > >>> total number of D state processes waiting to acquire the
> > > > > > >>> mutex lock will be greatly reduced, which otherwise were
> > > > > > >>> causing DB instance crash as the corresponding processes
> > > > > > >>> were not
> > > progressing while waiting to acquire the mutex lock.
> > > > > > >>>
> > > > > > >>> Signed-off-by: Praveen Kumar Kannoju
> > > > > > >>> <praveen.kannoju@xxxxxxxxxx> —
> > > > > > >>>
> > > > > > >> […]
> > > > > > >>
> > > > > > >>> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c index
> > > > > > >>> 8f070ee..6b640b5 100644
> > > > > > >>> +++ b/net/rds/ib_rdma.c
> > > > > > >>> @@ -393,6 +393,8 @@ int rds_ib_flush_mr_pool(struct
> > > rds_ib_mr_pool *pool,
> > > > > > >>> 	 */
> > > > > > >>> 	dirty_to_clean = llist_append_to_list(&pool->drop_list,
> > > &unmap_list);
> > > > > > >>> 	dirty_to_clean += llist_append_to_list(&pool->free_list,
> > > > > > >>> &unmap_list);
> > > > > > >>> +	WRITE_ONCE(pool->flush_ongoing, true);
> > > > > > >>> +	smp_wmb();
> > > > > > >>> 	if (free_all) {
> > > > > > >>> 		unsigned long flags;
> > > > > > >>>
> > > > > > >>> @@ -430,6 +432,8 @@ int rds_ib_flush_mr_pool(struct
> > > rds_ib_mr_pool *pool,
> > > > > > >>> 	atomic_sub(nfreed, &pool->item_count);
> > > > > > >>>
> > > > > > >>> out:
> > > > > > >>> +	WRITE_ONCE(pool->flush_ongoing, false);
> > > > > > >>> +	smp_wmb();
> > > > > > >>> 	mutex_unlock(&pool->flush_lock);
> > > > > > >>> 	if (waitqueue_active(&pool->flush_wait))
> > > > > > >>> 		wake_up(&pool->flush_wait); @@ -507,8 +511,17
> @@ void
> > > > > > >>> rds_ib_free_mr(void *trans_private, int
> > > > > > >>> invalidate)
> > > > > > >>>
> > > > > > >>> 	/* If we've pinned too many pages, request a flush */
> > > > > > >>> 	if (atomic_read(&pool->free_pinned) >= pool-
> > > >max_free_pinned ||
> > > > > > >>> -	    atomic_read(&pool->dirty_count) >= pool->max_items / 5)
> > > > > > >>> -		queue_delayed_work(rds_ib_mr_wq, &pool-
> > > >flush_worker, 10);
> > > > > > >>> +	    atomic_read(&pool->dirty_count) >= pool->max_items /
> > > > > > >>> +5)
> > > {
> > > > > > >>> +		smp_rmb();
> > > > > > >> You won’t need these explicit barriers since above atomic
> > > > > > >> and write once already issue them.
> > > > > > >
> > > > > > > No, they don't. Use smp_store_release() and smp_load_acquire
> > > > > > > if you want to do something like this, but I still can't
> > > > > > > quite figure out if this usage of unlocked memory accesses
> > > > > > > makes any
> > > sense at all.
> > > > > > >
> > > > > > Indeed, I see that now, thanks. Yeah, these multi variable
> > > > > > checks can indeed be racy but they are under lock at least for this
> code path.
> > > > > > But there are few hot path places where single variable states
> > > > > > are evaluated atomically instead of heavy lock.
> > > > >
> > > > > At least pool->dirty_count is not locked in rds_ib_free_mr() at all.
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Regards,
> > > > > > Santosh
> > > > > >
> > > > >
> > > > > Thank you Santosh, Leon and Jason for reviewing the Patch.
> > > > >
> > > > > 1. Leon, the bool variable "flush_ongoing " introduced through
> > > > > the patch
> > > has to be accessed only after acquiring the mutex lock. Hence it is
> > > well protected.
> > > >
> > > > I don't see any lock in rds_ib_free_mr() function where your
> > > > perform "if
> > > (!READ_ONCE(pool->flush_ongoing)) { ...".
> > > >
> > > > >
> > > > > 2. As the commit message already conveys the reason, the check
> > > > > being
> > > made in the function "rds_ib_free_mr" is only to avoid the redundant
> > > asynchronous workers from being spawned.
> > > > >
> > > > > 3. The synchronous flush path's through the function
> > > > > "rds_free_mr" with
> > > either cookie=0 or "invalidate" flag being set, have to be honoured
> > > as per the semantics and hence these paths have to execute the
> > > function "rds_ib_flush_mr_pool" unconditionally.
> > > > >
> > > > > 4. It complicates the patch to identify, where from the function
> > > "rds_ib_flush_mr_pool", has been called. And hence, this patch uses
> > > the state of the bool variable to stop the asynchronous workers.
> > > > >
> > > > > 5. We knew that "queue_delayed_work" will ensures only one work
> > > > > is
> > > running, but avoiding these async workers during high load
> > > situations, made way for the allocation and synchronous callers
> > > which would otherwise be waiting long for the flush to happen. Great
> > > reduction in the number of calls to the function "rds_ib_flush_mr_pool"
> has been observed with this patch.
> > > >
> > > > So if you understand that there is only one work in progress, why
> > > > do you
> > > say workerS?
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > 6. Jason, the only function "rds_ib_free_mr" which accesses the
> > > introduced bool variable "flush_ongoing" to spawn a flush worker
> > > does not crucially impact the availability of MR's, because the flush
> happens from
> > > allocation path as well when necessary.   Hence the Load-store ordering is
> > > not essentially needed here, because of which we chose smp_rmb() and
> > > smp_wmb() over smp_load_acquire() and smp_store_release().
> > > > >
> > > > > Regards,
> > > > > Praveen.
> > > >
> > > >
> > > > Jason,
> > > >
> > > > 	The relaxed ordering primitives smp_rmb() and smp_wmb() ensure
> > > to provide
> > > > 	guaranteed atomic memory operations READ_ONCE and
> > > WRITE_ONCE, used in the
> > > > 	functions "rds_ib_free_mr" and "rds_ib_flush_mr_pool"
> > > correspondingly.
> > > >
> > > > 	Yes, the memory barrier primitives smp_load_acquire()and
> > > smp_store_release()
> > > > 	are even better. But, because of the simplicity of the use of
> > > > memory
> > > barrier
> > > > 	in the patch, smp_rmb() and smp_wmb() are chosen.
> > > >
> > > > 	Please let me know if you want me to switch to smp_load_acquire()
> > > and
> > > > 	smp_store_release().
> > > >
> > > > Leon,
> > > >
> > > > 	Avoiding the asynchronous worker from being spawned during the
> > > high load situations,
> > > > 	make way for both synchronous and allocation path to flush the mr
> > > pool and grab the
> > > > 	mr without waiting.
> > > >
> > > > 	Please let me know if you still have any queries with this
> > > > respect or
> > > any modifications
> > > > 	are needed.
> > >
> > Thank you for your reply, Leon.
> >
> > > I didn't get any answer on my questions.
> > > So let's me repeat them.
> > > 1. Where can I see locks in rds_ib_free_mr() that protects
> > > concurrent change of your new flush_ongoing field?
> >
> > flush_ongoing variable is only modified in the function
> "rds_ib_flush_mr_pool" under mutex lock. It is only being read atomically in
> the function "rds_ib_free_mr()", with memory barrier in place.  Hence a lock
> is not essential in this function "rds_ib_free_mr()". Depending on the value
> being read, decision is taken weather to spawn the asynchronous worker or
> not.
> 
> This is not how locking works.
> 
>  CPU1                                     CPU2
>   rds_ib_flush_mr_pool
>   lock
>                         context switch -> rds_ib_free_mr
> 			                  READ old value of flush_ongoing
> 			<- context switch
>   WRITE new value to flush_ongoing
> 
> 
> >
> > > 2. There is only one same work can be executed/scheduled, where do
> > > you see multiple/parallel workers (plural) and how is it possible?
> >
> > In my earlier comment, I have re-iterated the same point. I would take
> back my word "workerS". By avoiding this asynchronous worker to
> participate in flushing, the synchronous flush jobs (cookie=0 and
> invalidate=1) as well as allocation path worker will be acquiring the mutex
> lock in the function "rds_ib_flush_mr_pool" quickly, thereby fetching the
> MR.
> 
> This is completely different scenario from what was presented before.
> You are interested to prioritize synchronous operations over async.
> In such case, why don't you simply cancel_delayed_work() in your sync
> flows?

Thank you, Leon. 
Will verify this approach and post the updated patch for review. 

> 
> >
> > I hope I am clear.
> >
> > > BTW, please fix your email client to reply inline.
> > Fixed it. Thank you.
> > >
> > > >
> > > > Regards,
> > > > Praveen.
> >
> >
> > Regards,
> > Praveen.