Question on ipmr.c locking in 2.6.25

Question on ipmr.c locking in 2.6.25

[Ben Greear]

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


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on ipmr.c locking in 2.6.25

[Ben Hutchings]

Ben Greear wrote:
> 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?
<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.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: Question on ipmr.c locking in 2.6.25

[Ben Greear]

Ben Hutchings wrote:
> Ben Greear wrote:
>> 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?
> <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).

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

> 
> Ben.
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on ipmr.c locking in 2.6.25

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Wed, 28 May 2008 15:06:45 -0700

> 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.