Re: [ATM] second pass at fixing atm spinlock

[chas williams]

In message <[e-mail]>,Duncan Sands writes:
>I'm kind of confused about this.  It seems to me that you should only
>need to read_lock(vcc_sklist_lock) if you are going to traverse (or
>otherwise examine the structure of) the list.  There should be no need

to prevent trouble with walking a list that might be changing while
you are walking it and to synchronize the release and "bottom half"
operation of the atm drivers.  i came up with this as the worst
possible case (of course its one of those lookup via big index table
drivers):

driver->open()		BH()		vcc_release()		sk.refcnt

ENTER
...
vcc_hold(vcc)								2
rx_vcc->vcc = vcc
...
EXIT
		ENTER
		read_lock(sklist)
		vcc = rx_vcc->vcc
		...			
					ENTER
					driver->close()
						...
						rx_vcc->vcc = NULL
						barrier()
						vcc_put(vcc)		1
					...
					write_lock(sklist) [MUST WAIT]
						
		vcc_hold(vcc)						2
		read_unlock(sklist)
		...
					...
					vcc_remove_socket()
					write_unlock(sklist)
					sock_put(vcc);			1
					EXIT

		...
		vcc->push(skb)
		vcc_put(vcc)						0
		EXIT

if you didnt read_lock(sklist) then vcc_release() could (it is somewhat
unlikely) unhash the socket and vcc_put() (its really just sock_put) before
the BH has a chance to vcc_hold(vcc).  the { vcc = rx_vcc->vcc; vcc_hold(vcc); }
isnt an atomic operation and that is were you run into trouble.  you
could remove the hold/put for for the table references and this still
works but putting the vcc in the table is a reference.  you should count it.
however, the nicstar/idt77252 probably still have a race since 
the vc && vc->vcc in the BH isnt protected.

>Why does this exist at all?  I mean, if someone has already opened a vcc for
>a given vpi/vci pair, the ATM layer could detect this itself and return an error,
>without ever calling the driver's open method.  Is it sometimes useful to open

mitch and i have had this discussion.  it seems like its probably a good
idea to move this to the upper layer and possibly remove the ATM_ANY_VPI/VCI
functionality.  it probably should be claim_ci, and insert the entry into
the list during operation.  as it stands there is still a race beween
find_ci() and vcc_insert_socket().