Re: [PATCH 01/13] kdbus: add documentation

Hi

On Mon, Jan 26, 2015 at 3:46 PM, Michael Kerrisk (man-pages)
[off-list ref] wrote:

Hello Greg,

On 01/23/2015 05:08 PM, Greg Kroah-Hartman wrote:

quoted

On Thu, Jan 22, 2015 at 09:49:00AM -0500, Austin S Hemmelgarn wrote:

quoted

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.

quoted

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.

Thanks
David