linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Herrmann <dh.herrmann@gmail.com>
To: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Austin S Hemmelgarn <ahferroin7@gmail.com>,
	Daniel Mack <daniel@zonque.org>, Arnd Bergmann <arnd@arndb.de>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>,
	Tom Gundersen <teg@jklm.no>, "Theodore T'so" <tytso@mit.edu>,
	Andy Lutomirski <luto@amacapital.net>,
	Linux API <linux-api@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Djalal Harouni <tixxdz@opendz.org>,
	Johannes Stezenbach <js@sig21.net>,
	Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 01/13] kdbus: add documentation
Date: Tue, 27 Jan 2015 16:05:20 +0100	[thread overview]
Message-ID: <CANq1E4SkHhs1pWUe-TzG7bzk1M-Q++mB2vmQGuYx0RMF53wg4Q@mail.gmail.com> (raw)
In-Reply-To: <54C65346.5070504@gmail.com>

Hi

On Mon, Jan 26, 2015 at 3:46 PM, Michael Kerrisk (man-pages)
<mtk.manpages@gmail.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.

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

  reply	other threads:[~2015-01-27 15:05 UTC|newest]

Thread overview: 86+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-16 19:16 [PATCH v3 00/13] Add kdbus implementation Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 01/13] kdbus: add documentation Greg Kroah-Hartman
2015-01-20 13:53   ` Michael Kerrisk (man-pages)
2015-01-20 14:31     ` David Herrmann
2015-01-20 14:42       ` Josh Boyer
2015-01-20 14:53         ` Djalal Harouni
2015-01-20 16:08           ` Johannes Stezenbach
2015-01-20 17:00             ` David Herrmann
2015-01-20 22:00               ` Johannes Stezenbach
2015-01-21 10:28       ` Michael Kerrisk (man-pages)
2015-01-20 18:23     ` Daniel Mack
2015-01-21 10:32       ` Michael Kerrisk (man-pages)
2015-01-21 15:19         ` Theodore Ts'o
2015-01-21 16:58         ` Daniel Mack
2015-01-22 10:18           ` Michael Kerrisk (man-pages)
2015-01-22 13:46             ` David Herrmann
2015-01-22 14:49               ` Austin S Hemmelgarn
2015-01-23 16:08                 ` Greg Kroah-Hartman
2015-01-26 14:46                   ` Michael Kerrisk (man-pages)
2015-01-27 15:05                     ` David Herrmann [this message]
2015-01-27 16:03                       ` Andy Lutomirski
2015-01-29  8:53                         ` Daniel Mack
2015-01-29 11:25                           ` Andy Lutomirski
2015-01-29 11:42                             ` Daniel Mack
2015-01-29 12:09                               ` Andy Lutomirski
2015-02-02  9:34                                 ` Daniel Mack
2015-02-02 20:12                                   ` Andy Lutomirski
2015-02-03 10:09                                     ` Daniel Mack
2015-02-04  0:41                                       ` Andy Lutomirski
2015-02-04  2:47                                         ` Eric W. Biederman
2015-02-04  3:14                                           ` Greg Kroah-Hartman
2015-02-04  6:30                                             ` Eric W. Biederman
2015-02-04 23:03                                       ` Andy Lutomirski
2015-02-05  0:16                                         ` David Herrmann
2015-02-08 16:54                                           ` Andy Lutomirski
2015-01-27 18:03                       ` Michael Kerrisk (man-pages)
2015-01-23 11:47               ` Michael Kerrisk (man-pages)
2015-01-23 15:54             ` Greg Kroah-Hartman
2015-01-26 14:42               ` Michael Kerrisk (man-pages)
2015-01-26 15:26                 ` Tom Gundersen
2015-01-26 16:44                   ` christoph Hellwig
2015-01-26 16:45                   ` Michael Kerrisk (man-pages)
2015-01-27 15:23                     ` David Herrmann
2015-01-27 17:53                       ` Michael Kerrisk (man-pages)
2015-01-27 18:14                         ` Daniel Mack
2015-01-28 10:46                           ` Michael Kerrisk (man-pages)
2015-01-20 13:58   ` Michael Kerrisk (man-pages)
2015-01-20 17:50     ` Daniel Mack
2015-01-21  8:57       ` Michael Kerrisk (man-pages)
2015-01-21  9:07         ` Daniel Mack
2015-01-21  9:07     ` Michael Kerrisk (man-pages)
2015-01-21  9:12       ` Daniel Mack
2015-01-23  6:28   ` Ahmed S. Darwish
2015-01-23 13:19     ` Greg Kroah-Hartman
2015-01-23 13:29       ` Greg Kroah-Hartman
2015-01-25  3:30       ` Ahmed S. Darwish
2015-01-16 19:16 ` [PATCH 02/13] kdbus: add header file Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 03/13] kdbus: add driver skeleton, ioctl entry points and utility functions Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 04/13] kdbus: add connection pool implementation Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 05/13] kdbus: add connection, queue handling and message validation code Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 06/13] kdbus: add node and filesystem implementation Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 07/13] kdbus: add code to gather metadata Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 08/13] kdbus: add code for notifications and matches Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 09/13] kdbus: add code for buses, domains and endpoints Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 10/13] kdbus: add name registry implementation Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 11/13] kdbus: add policy database implementation Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 12/13] kdbus: add Makefile, Kconfig and MAINTAINERS entry Greg Kroah-Hartman
2015-01-16 19:16 ` [PATCH 13/13] kdbus: add selftests Greg Kroah-Hartman
2015-01-16 22:07 ` [PATCH v3 00/13] Add kdbus implementation Josh Boyer
2015-01-16 22:18   ` Greg Kroah-Hartman
2015-01-17  0:26     ` Daniel Mack
2015-01-17  0:41       ` Josh Boyer
2015-01-19 18:06 ` Johannes Stezenbach
2015-01-19 18:38   ` Greg Kroah-Hartman
2015-01-19 20:19     ` Johannes Stezenbach
2015-01-19 20:31       ` Greg Kroah-Hartman
2015-01-19 23:38         ` Johannes Stezenbach
2015-01-20  1:13           ` Greg Kroah-Hartman
2015-01-20 10:57             ` Johannes Stezenbach
2015-01-20 11:26               ` Greg Kroah-Hartman
2015-01-20 13:24                 ` Johannes Stezenbach
2015-01-20 14:12                   ` Michael Kerrisk (man-pages)
2015-01-26 21:32             ` One Thousand Gnomes
2015-01-19 18:33 ` Johannes Stezenbach
2015-01-20 14:05 ` Michael Kerrisk (man-pages)
2015-01-20 14:15 ` Michael Kerrisk (man-pages)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANq1E4SkHhs1pWUe-TzG7bzk1M-Q++mB2vmQGuYx0RMF53wg4Q@mail.gmail.com \
    --to=dh.herrmann@gmail.com \
    --cc=ahferroin7@gmail.com \
    --cc=arnd@arndb.de \
    --cc=daniel@zonque.org \
    --cc=ebiederm@xmission.com \
    --cc=gnomes@lxorguk.ukuu.org.uk \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=js@sig21.net \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mtk.manpages@gmail.com \
    --cc=teg@jklm.no \
    --cc=tixxdz@opendz.org \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).