Re: [PATCH for-next V5 00/12] Move RoCE GID management to IB/Core

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Fri, Jun 12, 2015 at 03:29:25PM +0300, Or Gerlitz wrote:

> I understand the email thread went down into further details from this
> point and on, but still, I'd like to stop here and ask for short
> clarification -- my understanding of things re this reader-writer
> scheme is the following:
> 
> 1. some reader/s can't be made to sleep as they make calls in atomic context
> and hence it wouldn't be correct to use RW semaphore
> 
> 2. some writers go to sleep (invoke driver call to the firmware) and
> hence they can't be holding RW spinlock
> 
> Agree? if not, can you shortly say what's wrong in these assumptions,
> or I should just deeply read the thread.

Sure, broadly.

> B/c if these assumptions are correct, seqlock/seqcount seem to me very
> much like the right approach. It addresses the constraints 1 and 2
> above, simple/robust to implement/use

Well, it is anything but simple. There are two subtle complex things
going on with this use of seqlock:
 - If you hold a seqlock while sleeping and ignore the read-retry
   mechanism then upon update readers see three states: NEW,OLD,INVALID
- It is impossible to dev_hold attr->ndev without using RCU

You read the patch, did you understand those subtle details were
happening? It took me a few re-reads to get it..

Far better is a simple spin lock:

lock(table->lock)
spin_lock()
attr = ...;
invalid = 1;
spin_unlock();

driver->update_gid

spin_lock()
invalid = 0;
spin_unlock();
unlock(table->lock)

Which solves *exactly* the same problem and doesn't require RCU to
extract the ndev. [In this case, I think the invalid state is actually
undesirable, so I'd work to eliminate it]

I keep being suprised by this insistance on performance locking.  99%
of the time the performance locks and simple locks are
interchangeable. If you reach a situation where you think you *NEED* a
performance lock and can't see how to do it with a simple lock, almost
certainly, you are doing something wrong.

So as a reviewer, asking for simple locks should get the author to
consider their locking carefully and either produce something correct
with simple locks or solve the issues with their performance locks,
independently.

Especially with RCU, there is no excuse, there is a RCU review
checklist, and both Matan and Yishai's v1 patches *blatantly* fail points
in that list. 'RCU is used wrong, read the checklist' is all a reviewer
should have to say.

Being drawing into a stupid argument is pointless, and in future I'm
going to stop taking the bait. :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html