Re: [ATM] first pass at fixing atm spinlock

[Mitchell Blank Jr <mitch@sfgoth.com>]

chas williams wrote:
> ftp://ftp.cmf.nrl.navy.mil/pub/chas/linux-atm/2_5_64_atm_dev_lock.patch

Note that this patch also has lots of other stuff (he driver, ipv6 stuff)

I'll try to look at the patch in depth later, but I'll make a few initial
comments.  First of all, thanks for taking this up - this is work I've wanted
to do for years and never found the time for.

> atm_dev_lock is now a
> read/write lock that now protects the list of atm devices.

Are you sure you're never sleeping while holding this lock while iterating
device lists?  I haven't checked but we do a fair number of things there
(like the proc stuff) so we really need to track those down.

> things are not set in stone yet, so i would like some advice -- the per 
> device vcc list will probably be traversed (read-only) in the bottom
> half of the kernel during receive for some adapters.

Yes, absolutely.

> is it simply
> enough to use list_for_each_safe() when traversing the list?  otherwise
> the per device spinlock will need to shared between the top and bottom half
> of the kernel.  the vcc's shouldnt need to be ref counted since they
> are rather tightly coupled to a struct sock which is.

Then you should implement functions to grab/release the "struct sock"'s
refernce count based on the atm_vcc.  Then you'd have 
  atm_vcc_find(dev, vpi, vci)
  atm_vcc_hold(vcc)
  atm_vcc_release(vcc)
Just like that atm_dev_*() functions.  Otherwise there's no reason that the
sock/vcc combo can't go away while a processor's bh still is using a reference
to the vcc.  I think this has been the result of many of the reported SMP
crashes (it's probably not that hard to trigger; just close an ATM socket
that's receiving a flood of traffic)

> i dont like
> the 'shutdown on last reference' in atm_dev_release().  you can only
> safely call release when you know you can sleep.  is shutdown_atm_dev()
> really that useful?

Yes, it's needed (especially if you want to support more complicated interfaces
like ADSL cards via the ATM stack in the future)

Really the dev release stuff should ONLY be called from the close() path.
You really need something like atm_dev_release_last() that waits for the
reference count to hit "1" (via a completion or something) and then does
the release stuff.

-Mitch