Re: [ATM] second pass at fixing atm spinlock

From: chas williams (chas@locutus.cmf.nrl.navy.mil)
Date: Fri Mar 28 2003 - 08:08:37 EST


In message <200303281107.07586.baldrick@wanadoo.fr>,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().
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Mon Mar 31 2003 - 22:00:32 EST