Question on ipmr.c locking in 2.6.25
Subsystems (2)
It looks like this method can return without unlocking the mrt_lock or mfc_unres_lock. Is this a bug, or am I just confused about how it is supposed to work?
I haven't reproduced any problem..just staring at the code while trying to add support for multiple routing tables...
static struct mfc_cache *ipmr_mfc_seq_idx(struct ipmr_mfc_iter *it, loff_t pos)
{
struct mfc_cache *mfc;
it->cache = mfc_cache_array;
read_lock(&mrt_lock);
for (it->ct = 0; it->ct < MFC_LINES; it->ct++)
for (mfc = mfc_cache_array[it->ct]; mfc; mfc = mfc->next)
if (pos-- == 0)
return mfc;
read_unlock(&mrt_lock);
it->cache = &mfc_unres_queue;
spin_lock_bh(&mfc_unres_lock);
for (mfc = mfc_unres_queue; mfc; mfc = mfc->next)
if (pos-- == 0)
return mfc;
spin_unlock_bh(&mfc_unres_lock);
it->cache = NULL;
return NULL;
}Thanks,
Ben
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 3 lines · Ben Greear
It looks like this method can return without unlocking the
mrt_lockormfc_unres_lock. Is this a bug, or am I just confused about how it is supposed to work?
<snip>
Since it returns without unlocking in the normal (not error) case, I would guess that's how it's supposed to work. The caller can uses the it->cache pointer to work out which lock (if any) it needs to unlock.
Still, this is an unusual way of doing things, and rates about a 2 on Rusty's scale <http://ozlabs.org/~rusty/index.cgi/2008/03/30/> (though to be fair this is not critical for static functions).
Ben.
Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job.
Quoted 6 lines · Ben Hutchings
Quoted 3 lines · Ben Greear
It looks like this method can return without unlocking the
mrt_lockormfc_unres_lock. Is this a bug, or am I just confused about how it is supposed to work?<snip>
Quoted 7 lines
Since it returns without unlocking in the normal (not error) case, I would guess that's how it's supposed to work. The caller can uses the
it->cachepointer to work out which lock (if any) it needs to unlock.Still, this is an unusual way of doing things, and rates about a 2 on Rusty's scale <http://ozlabs.org/~rusty/index.cgi/2008/03/30/> (though to be fair this is not critical for static functions).
Ok, I think I see how it works.
Now I wonder: If a reader read only a small bit of the proc file, and then just went to sleep w/out closing or reading the rest of the file, would that effectively DOS a system by pinning the locks?
Thanks,
Ben
Quoted 3 lines
Ben.
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
David Miller
maintainerFrom: Ben Greear <[email protected]>
Date: Wed, 28 May 2008 15:06:45 -0700
Quoted 3 lines
Now I wonder: If a reader read only a small bit of the proc file, and then just went to sleep w/out closing or reading the rest of the file, would that effectively DOS a system by pinning the locks?
That's not how this works.
The in-kernel buffer used by the SEQ file infrastructure is filled as best as possible. Then the locks are dropped and the copy to userspace is performed.
Since we hold locks, this also means preemption is disabled. So there is no way for the task to go to sleep at this time.