flock(fd, LOCK_UN) taking 500ms+ ?

In 2.5.40, our application is getting seemingly very large flock(LOCK_UN) latencies :

Unlock, took 541386 usecs
Old was 567999237 secs, 131042 usecs now is 567999237 secs, 672428 usecs Unlock, took 125083 usecs
Old was 567999237 secs, 699310 usecs now is 567999237 secs, 824393 usecs Unlock, took 151245 usecs
Old was 567999237 secs, 825119 usecs now is 567999237 secs, 976364 usecs

(using gettimeofday ...)

No similar times are observed during the corresponding LOCK_EX call.

Is this just a silly app bug somewhere, or something funny in locks.c ?

More details, testing etc. on request ...

regards
john

"I never understood what's so hard about picking a unique
 first and last name - and not going beyond the 6 character limit."
 	- Toon Moene

Hmm. This :

@@ -727,11 +727,11 @@
 	}
 	unlock_kernel();
 
-	if (found)
-		yield();
-
 	if (new_fl->fl_type == F_UNLCK)
 		return 0;
+
+	if (found)
+		yield();
 
 	lock_kernel();
 	for_each_lock(inode, before) {

"fixes" the problem (a simultaneous kernel compile is going on; it's a 2-way SMP machine). What is the yield for ? What's the right fix (if any) ? This turns our app's main loop from a second or two into a 90-second behemoth.

There are no other apps taking locks on the relevant files, btw

regards
john

"I never understood what's so hard about picking a unique
 first and last name - and not going beyond the 6 character limit."
 	- Toon Moene

I'm pretty sure this is correct. There's no particular reason to yield() if we're unlocking.

I wonder if yield() is the correct call to make anyway. We certainly need to schedule() to allow any higher-priority task the opportunity to run. But if we're the highest-priority task downgrading our write-lock to a read-lock which permits other tasks the opportunity to run, i see no reason we should _yield_ to them.

Scheduling is a bit of a black art as far as I'm concerned. Someone with a bit more experience care to comment?

Revolutions do not require corporate support.

sched_yield() sementics changed a lot. It used to mean "take a quick nap", but it now means "go to the back of the runqueue and stay there for absolutely ages". The latter is a closer interpretation of the spec, but it has broken stuff which was tuned to the old behaviour.

It's not really clear why that yield is in there at all? Unless that code is really, really slow (milliseconds) then probably it should just be deleted.

If there really is a solid need to hand the CPU over to some now-runnable higher-priority process then a cond_resched() will suffice.

*nod*. This code has been around for many many years ;-)

Heh, you're so focused on perf tuning, Andrew! It's not a matter of locking, it's a matter of semantics. Here's the comment:

 *  FL_FLOCK locks never deadlock, an existing lock is always removed before
 *  upgrading from shared to exclusive (or vice versa). When this happens
 *  any processes blocked by the current lock are woken up and allowed to
 *  run before the new lock is applied.
 *  Andy Walker ([email protected]), June 09, 1995

I think that's the right thing to do. If I understand right, we'll check needs_resched at syscall exit, so we don't need to do it for unlocks, right?

Revolutions do not require corporate support.

How will cond_resched() work ? Surely that will only give a chance if the current process has reached the end of its timeslice (need_resched) ? Isn't "schedule()" the right thing here ?

right ...

regards
john

"Me and my friends are so smart, we invented this new kind of art:
 Post-modernist throwing darts"
	- the Moldy Peaches

need_resched is set whenever there is a higher priority process that is now runnable (it is set on wake_up()). Otherwise cond_resched() would be fairly worthless.. and kernel preemption, etc.

Robert Love

Oh, I understand now. In this case, you actually do need to call yield() as gross as it is.

You have four options:

- call schedule(). This will result in the highest priority task running again which may be you. You will certainly end up running again in the near future and there may be processes blocked by the current lock which will not run before you continue.

- call cond_resched(). Same as above but avoid the superfluous reschedule back to yourself, since if need_resched is unset we won't call schedule().

- call yield(). You are put at the end of the runqueue and thus all runnable tasks should run before you get to run again.

- do nothing. Always my choice :)

So yield() is the only way to guarantee that all lock holders run before you do (it does not even do that, however: it is possible for you to get reinserted into the active array and thus it only guarantees that all lock holders at or above your priority get to run before you). cond_resched() will guarantee all higher priority tasks run before you.

If you REALLY want to assure you do not run at all until all the other lock holders ran, you would need to down() a semaphore and not wake up until all of them have run again (I have no idea how the flock code looks, if this is even feasible...).

Right. On return to user-space need_resched will be checked and schedule() called if it is set.

However, it is only set if the newly woken up tasks have a higher priority than you. Otherwise, schedule() would just select you again.

Robert Love

hm. This is a tricky thing to guarantee. If this process is high-priority or SCHED_RR or whatever, we want to ensure that any current holder of the lock gets a CPU slice?

Seems a strange thing to want to do, and if we really want to implement these semantics then there's quite a bit of stuff to do - making *all* blocked processes get some CPU will involve scheduler work, or funny games with semaphores.

Now if we interpret "allowed to run" as meaning "made runnable" then no probs. Just wake them up.

Sure. Sounds to me like we just want to delete the code ;)

Yeah, I think the original author was a little imprecise in his description of the semantics. The freebsd flock(2) manpage says:

A shared lock may be upgraded to an exclusive lock, and vice versa, sim­ ply by specifying the appropriate lock type; this results in the previous lock being released and the new lock applied (possibly after other pro­ cesses have gained and released the lock).

So I think what they're trying to say is that changing the lock type is exactly equivalent to removing the existing lock and then applying the new lock; it just happens to be one syscall. Using cond_resched() looks like the right approach.

Revolutions do not require corporate support.