Re: [PATCH] Replace completions with semaphores

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Matthew Wilcox
Date: Saturday, April 12, 2008 - 10:26 am

On Sat, Apr 12, 2008 at 02:24:41PM +0200, Peter Zijlstra wrote:

OK, so all three of you have got the wrong end of the stick.

My point is that the struct semaphore and the struct completion are
essentially the same data structure, operated on in essentially the same
way and having essentially the same purpose.

It would look bloody odd to write (code taken from megasas_mgmt_ioctl_fw() in
drivers/scsi/megaraid/megaraid_sas.c):

        if (wait_for_completion_interruptible(&instance->ioctl_completion)) {
                error = -ERESTARTSYS;
                goto out_kfree_ioc;
        }
        error = megasas_mgmt_fw_ioctl(instance, user_ioc, ioc);
        complete(&instance->ioctl_sem);

What I'm trying to get a feeling for is whether people find it similarly
odd to use semaphores where we currently use completions.  We *used*
to, but I don't find that a compelling reason.


Arnd contacted me off-list and made the very sensible suggestion of:

struct completion {
	struct semaphore sem;
}

That lets us eliminate the duplicate code since all the completion
functions become very thin wrappers around semaphore operations.

I'll note that the semaphore code I hae queued for 2.6.26 is slightly
more efficient than the current implementation of completions because
completions use the generic waitqueue code and thus do an indirect
function call per wakeup.  Of course, there's no reason completions
couldn't use the same technique as my semaphore code ... but then they
would be identical to semaphores ;-)

So I'm going to redo my patch using Arnd's idea because it allows us
to eliminate code without changing the API at all.  We can continue the
discussion about eliminating one or the other API, but my opinion is that
eliminating either is a mistake.  The choice of API indicates how the
author thinks about the code, which is crucial for those reading the code.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[PATCH] Replace completions with semaphores, Matthew Wilcox, (Fri Apr 11, 2:00 pm)
Re: [PATCH] Replace completions with semaphores, Daniel Walker, (Fri Apr 11, 11:43 pm)
Re: [PATCH] Replace completions with semaphores, Ingo Oeser, (Sat Apr 12, 3:31 am)
Re: [PATCH] Replace completions with semaphores, Peter Zijlstra, (Sat Apr 12, 5:24 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Sat Apr 12, 10:26 am)
Re: [PATCH] Replace completions with semaphores, Daniel Walker, (Sat Apr 12, 11:01 am)
Re: [PATCH] Replace completions with semaphores, Peter Zijlstra, (Sat Apr 12, 11:05 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Sat Apr 12, 12:04 pm)
Re: [PATCH] Replace completions with semaphores, Peter Zijlstra, (Sat Apr 12, 12:16 pm)
Re: [PATCH] Replace completions with semaphores, Roland Dreier, (Sat Apr 12, 12:53 pm)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Sat Apr 12, 1:47 pm)
Re: [PATCH] Replace completions with semaphores, Ingo Molnar, (Sun Apr 13, 12:05 am)
Re: [PATCH] Replace completions with semaphores, Ingo Molnar, (Sun Apr 13, 12:08 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Sun Apr 13, 5:52 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Sun Apr 13, 5:57 am)
Re: [PATCH] Replace completions with semaphores, Bart Van Assche, (Sun Apr 13, 6:55 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Sun Apr 13, 7:22 am)
Re: [PATCH] Replace completions with semaphores, Bart Van Assche, (Sun Apr 13, 7:55 am)
Re: [PATCH] Replace completions with semaphores, Ingo Molnar, (Mon Apr 14, 8:39 am)
Re: [PATCH] Replace completions with semaphores, Ingo Molnar, (Mon Apr 14, 8:41 am)
Re: [PATCH] Replace completions with semaphores, Roland Dreier, (Mon Apr 14, 8:58 am)
Re: [PATCH] Replace completions with semaphores, Peter Zijlstra, (Mon Apr 14, 9:32 am)
Re: [PATCH] Replace completions with semaphores, Jens Axboe, (Mon Apr 14, 9:54 am)
Re: [PATCH] Replace completions with semaphores, Arjan van de Ven, (Mon Apr 14, 9:56 am)
Re: [PATCH] Replace completions with semaphores, Andi Kleen, (Mon Apr 14, 10:46 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Mon Apr 14, 10:46 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Mon Apr 14, 10:50 am)
Re: [PATCH] Replace completions with semaphores, Peter Zijlstra, (Mon Apr 14, 10:54 am)
Re: [PATCH] Replace completions with semaphores, Daniel Walker, (Mon Apr 14, 11:09 am)
Re: [PATCH] Replace completions with semaphores, Andi Kleen, (Mon Apr 14, 12:16 pm)
Re: [PATCH] Replace completions with semaphores, Alan Cox, (Mon Apr 14, 12:16 pm)
Re: [PATCH] Replace completions with semaphores, Bart Van Assche, (Mon Apr 14, 11:18 pm)
Re: [PATCH] Replace completions with semaphores, Peter Zijlstra, (Mon Apr 14, 11:46 pm)
Re: [PATCH] Replace completions with semaphores, Bart Van Assche, (Tue Apr 15, 12:17 am)
Re: [PATCH] Replace completions with semaphores, Peter Zijlstra, (Tue Apr 15, 1:44 am)
Re: [PATCH] Replace completions with semaphores, Bart Van Assche, (Tue Apr 15, 6:15 am)
Re: [PATCH] Replace completions with semaphores, Linus Torvalds, (Tue Apr 15, 9:09 am)
Re: [PATCH] Replace completions with semaphores, Andi Kleen, (Tue Apr 15, 9:27 am)
Re: [PATCH] Replace completions with semaphores, Linus Torvalds, (Tue Apr 15, 9:57 am)
Re: [PATCH] Replace completions with semaphores, Ingo Molnar, (Tue Apr 15, 10:05 am)
Re: [PATCH] Replace completions with semaphores, Andi Kleen, (Tue Apr 15, 10:15 am)
Re: [PATCH] Replace completions with semaphores, Linus Torvalds, (Tue Apr 15, 10:26 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Tue Apr 15, 10:41 am)
Re: [PATCH] Replace completions with semaphores, Linus Torvalds, (Tue Apr 15, 11:14 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Tue Apr 15, 11:50 am)
Re: [PATCH] Replace completions with semaphores, Ingo Molnar, (Wed Apr 16, 5:37 am)
Re: [PATCH] Replace completions with semaphores, Andi Kleen, (Wed Apr 16, 5:50 am)
Killable stat/readdir, Matthew Wilcox, (Wed Apr 16, 5:59 am)
Re: [PATCH] Replace completions with semaphores, Ingo Oeser, (Wed Apr 16, 9:07 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Wed Apr 16, 9:16 am)
Re: [PATCH] Replace completions with semaphores, Oliver Neukum, (Wed Apr 16, 9:31 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Wed Apr 16, 9:34 am)
Re: [PATCH] Replace completions with semaphores, Oliver Neukum, (Wed Apr 16, 9:42 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Wed Apr 16, 9:44 am)
Re: [PATCH] Replace completions with semaphores, Roland Dreier, (Wed Apr 16, 9:47 am)
Re: [PATCH] Replace completions with semaphores, Arjan van de Ven, (Wed Apr 16, 9:50 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Wed Apr 16, 9:58 am)
Re: [PATCH] Replace completions with semaphores, Arjan van de Ven, (Wed Apr 16, 10:08 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Wed Apr 16, 10:12 am)
Re: [PATCH] Replace completions with semaphores, Matthew Wilcox, (Wed Apr 16, 11:10 am)