Message-ID: <54C7D300.8070208@gmail.com>
Date:	Tue, 27 Jan 2015 19:03:44 +0100
From:	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
To:	David Herrmann <dh.herrmann@...il.com>
CC:	mtk.manpages@...il.com,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Austin S Hemmelgarn <ahferroin7@...il.com>,
	Daniel Mack <daniel@...que.org>, Arnd Bergmann <arnd@...db.de>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Tom Gundersen <teg@...m.no>, Theodore T'so <tytso@....edu>,
	Andy Lutomirski <luto@...capital.net>,
	Linux API <linux-api@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Djalal Harouni <tixxdz@...ndz.org>,
	Johannes Stezenbach <js@...21.net>,
	Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH 01/13] kdbus: add documentation

Hi David,

On 01/27/2015 04:05 PM, David Herrmann wrote:
> Hi
> 
> On Mon, Jan 26, 2015 at 3:46 PM, Michael Kerrisk (man-pages)
> <mtk.manpages@...il.com> wrote:
>> Hello Greg,
>>
>> On 01/23/2015 05:08 PM, Greg Kroah-Hartman wrote:
>>> On Thu, Jan 22, 2015 at 09:49:00AM -0500, Austin S Hemmelgarn wrote:
>>>> While I agree that there should be a way for userspace to get the list of
>>>> supported operations, userspace apps will only actually care about that
>>>> once, when they begin talking to kdbus, because (ignoring the live kernel
>>>> patching that people have been working on recently) the list of supported
>>>> operations isn't going to change while the system is running.  While a u64
>>>> copy has relatively low overhead, it does have overhead, and that is very
>>>> significant when you consider part of the reason some people want kdbus is
>>>> for the performance gain.  Especially for those automotive applications that
>>>> have been mentioned which fire off thousands of messages during start-up,
>>>> every little bit of performance is significant.
>>>
>>> A single u64 in a structure is not going to be measurable at all,
>>> processors just copy memory too fast these days for 4 extra bytes to be
>>> noticable.
>>
>> It depends on the definition of measurable, I suppose, but this statement
>> appears incorrect to me. In some cases (e.g., kdbus_msg_info) we're talking
>> about *two* u64 fields (kernel_gs, kernel_msg_flags) being used to pass back
>> sets of valid flags. That's 16 bytes, and it definitely makes a difference.
>> Simply running a loop that does a naive memcpy() in a tight user-space
>> loop (code below), I see the following for the execution of 1e9 loops:
>>
>>     Including the two extra u64 fields: 3.2 sec
>>     Without the two extra u64 fields:   2.6 sec
>>
>> On the same box, doing 1e9 calls to getppid() (i.e., pretty much the
>> simplest syscall, giving us a rough measure of the context switch) takes
>> 68 seconds. In other words, the cost of copying those 16 bytes is about 1%
>> of the base context switch/syscall cost. I assume the costs of copying
>> those 16 bytes across the kernel-user-space boundary would not be cheaper,
>> but have not tested that. If my assumption is correct, then 1% seems a
>> significant figure to me in an API whose raison d'être is speed.
> 
> I have no idea how this is related to any kdbus ioctl?
> 
> A 16byte copy does not affect the performance of kdbus message
> transactions in any way that matters.

I'm not sure if it's related/significant or not, since I'm ignorant
of the performance figures for kdbus. I just got curious when Greg
stated that the cost of copying would not be noticeable. (I got curious 
also about my assumption, and did an experiment with a dummy system call
that throws bytes across the fence into user space. The cost of an
extra 16 bytes (56 to 72 bytes) is about 3% of the base syscall/context 
switch cost.)

>>> So let's make this as easy as possible for userspace, making
>>> it simpler logic there, which is much more important than saving
>>> theoretical time in the kernel.
>>
>> But this also missed the other part of the point. Copying these fields on
>> every operation, when in fact they are only needed once, clutters the API,
>> in my opinion. Good APIs are as simple as they can be to do their job.
>> Redundancy is an enemy of simplicity. Simplest would have been a one time
>> API that returns a structure containing all of the supported flags across
>> the API. Alternatively, the traditional EINVAL approach is well understood,
>> and suffices.
> 
> We're going to drop "kernel_flags" in favor of a new
> KDBUS_FLAG_NEGOTIATE flag which asks the kernel to do feature
> negotiation for this ioctl and return the supported flags/items inline
> (overwriting the passed data). The ioctl will not be executed and will
> not affect the state of the FD.
> I hope this keeps the API simple.

Not sure I quite understand the details from your description, but I assume 
the it'll end up in the doc, and I'll try to take a look later.

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists