Re: shared variables between requester and completer threads - a concern

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jun 15, 2023 at 11:01:38AM -0500, Bob Pearson wrote:
> I am still on a campaign to tighten the screws in the rxe driver. There are a lot of variables that are shared
> between the requester task and the completer task (now on work queues) that control resources and error recovery.
> There is almost no effort to make sure changes in one thread are visible in the other. The following is a summary:
> 
> 				In requester task		In completer task
> 	qp->req.psn			RW				R
> 	qp->req.rd_atomic (A)		RW				W
> 	qp->req.wait_fence		W				RW
> 	qp->req.need_rd_atomic		W				RW
> 	qp->req.wait_psn		W				RW
> 	qp->req.need_retry		RW				RW
> 	qp->req.wait_for_rnr_timer	RW				W
> 
> These are all int's except for rd_atomic which is an atomic_t and all properly aligned.
> Several of these are similar to wait_fence:
> 
> 				if (rxe_wqe_is_fenced(qp, wqe) {
> 					qp->req.wait_fence = 1;
> 					goto exit; (the task thread)
> 				}
> 						...
> 								// completed something
> 								if (qp->req.wait_fence) {
> 									qp->req.wait_fence = 0;
> 									rxe_sched_task(&qp->req.task);
> 									// requester will run at least once
> 									// after this
> 								}
> 
> As long as the write and read actually get executed this will work eventually because the caches are
> coherent. But what if they don't? The sched_task implies a memory barrier before the requester task
> runs again but it doesn't read wait_fence so it doesn't seem to matter.
> 
> There also may be a race between a second execution of the requester re-setting the flag and the completer
> clearing it since someone else (e.g. verbs API could also schedule the requester.) I think the worst
> that can happen here is an extra rescheduling which is safe.
> 
> Could add an explicit memory barrier in the requester or matched smp_store_release/smp_load_acquire,
> or a spinlock, or WRITE_ONCE/READ_ONCE. I am not sure what, if anything, should be done in this case.
> It currently works fine AFAIK on x86/x64 but there are others.

It looks really sketchy.

This is the requestor hitting a fence opcode and needing to pause
processing until the completor reaches the matching barrier? How is
this just not completely racy? Forget about caches and barriers.

Jason