Re: [PATCH v2 08/11] firewire-sbp-target: Add sbp_login.{c,h}

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

On Feb 16 Chris Boot wrote:
> The session is only really inspected/maniuplated in the context of work 
> items, so it's possible I could use mutexes - although in the target 
> agent address handler I do check the node_id against the one in the 
> session (and I should really check the generation too). Would memory 
> barriers be enough for those or would locking be required?

A node ID itself is an incomplete datum.  Only a card--generation--node_ID
tuple is complete.

If generation and node ID are written and read concurrently, you have a
couple of choices to do this consistently:  You could atomically write and
read an integer which contains 8 bits of generation and 16 bits of node
ID.  Or you could serialize write and read accesses by a lock or mutex.
Or you could force the write side to write node ID before generation
(with an smp_wmb between to enforce order), and the read side to read
generation before node ID (with an smp_rmb to enforce order).

The latter method leaves a brief window in which a reader could pick up a
current node ID together with an outdated generation.  This is harmless
because the OHCI hardware and respective precautions in firewire-ohci
prevents transaction subactions with outdated generation to go out to the
wire.

It is generally recommended to implement such accesses in drivers with
locks (spinlocks or mutexes).  Among the various means to deal with
concurrency issues, locks seem to be best understood by us simple folk who
hack on drivers. :-)

[...]
> I guess I need to protect access to 
> the entire session really. Possibly even rwlocks due to only the 
> management processes ever changing anything, but lots of reads during 
> command handling.

The use case for rwlocks is not really the case where infrequent write
access meets frequent read access.  Rather, the use case is when it is
important to reduce or prevent contention between readers.

Rwlocks come with their own downsides though.  I guess the somewhat
costlier lock implementation could counter the benefit of allowing
concurrent readers.  Or maybe latency spikes around a write access could be
an issue.

I believe I have read somewhere that one should rather use a simple
spinlock unless exhaustive tests prove that an rwlock really performs
better.  Furthermore, in many if not all use cases of rwlocks, RCU is
available as another alternative.  RCU comes with its own set of downsides
though, for example not being as well and widely understood by programmers
compared to locking, being less easy to debug (may have improved
recently), and posing some challenges to RT-PREEMPT kernels.

AFAIU the above considerations cannot be applied 100 % to able-to-sleep
reader-writer locks, i.e. the kernel's rwsem.  Still, the use case of an
rwsem (in contrast to a mutex) is not particularly where a datum is rarely
written and often read, but where it is desirable to let multiple readers
not block each other.

[Somebody correct me where I'm wrong.]

PS, I cloned your git tree not long ago, but again various distractions
kept me from having a broader look at your code...
-- 
Stefan Richter
-=====-===-- --== ---==
http://arcgraph.de/sr/