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

[Leon Romanovsky]

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?

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