Date:	Thu, 23 Apr 2015 19:16:40 +0200
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.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>, Jiri Kosina <jkosina@...e.cz>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Daniel Mack <daniel@...que.org>,
	David Herrmann <dh.herrmann@...il.com>,
	Djalal Harouni <tixxdz@...ndz.org>
Subject: Re: [GIT PULL] kdbus for 4.1-rc1

On Thu, Apr 23, 2015 at 09:46:22AM -0700, Andy Lutomirski wrote:
> On Thu, Apr 23, 2015 at 9:36 AM, Greg Kroah-Hartman
> <gregkh@...uxfoundation.org> wrote:
> > On Thu, Apr 23, 2015 at 03:05:48PM +0200, Greg Kroah-Hartman wrote:
> >>
> >> Andy's concerns about the capability stuff has been hashed out in
> >> multiple threads here.  The kernel code isn't buggy as-designed or
> >> implemented from what we can all tell, it's just that the new
> >> functionality isn't liked by everyone, which is totally fair, but not a
> >> reason to declare that the function isn't useful.
> >
> > Andy, did I capture your existing position correctly?  If we drop the
> > caps metadata, I'm guessing that you are ok with the code as you have
> > reviewed it and tested it out.  So should I just add a small patch that
> > removes this for now?  After that, we can discuss the addition of
> > capabilities to the metadata as an add-on feature with a future patch
> > and not hold up this larger merge request?
> 
> No.  I can fish out lists I've posted of what I personally dislike.
> To repeat from my not-yet-awake memory, briefly:
> 
>  - starttime, cmdline, and possibly other pieces of metadata are also
> problematic.  I think starttime is especially bad because it both
> breaks CRIU and is IMO completely unnecessary -- I sent out draft
> "highpid" patches a while ago to give a much better alternative that
> isn't racy and won't break CRIU.  But cmdline is also IMO ridiculous.

starttime was removed a while ago, are you sure you are looking at the
latest code?

cmdline has been discussed and it really helps with debugging.
Decisions aren't being made based on it.

>  - There's still an open performance question.  Namely: is kdbus performant?

Yes, I thought that was already answered.  Tizen posted some numbers
with a much older version of the code, before David fixed a bunch of
issues that he and you found, and that averaged between 25-50% faster.
Details are in this presentation:
	http://download.tizen.org/misc/media/conference2014/slides/tdc2014-kdbus-in-tizen3.pdf

The Tizen and GENIVI developers are off running numbers with the latest
code, or so they told me through emails, but I don't know when/if that
will ever happen, so I can't promise more than what is already here.

>  - The policy system still sucks.  Now, if we give up on the idea of
> anyone ever using it for anything other than dbus as it currently
> works, maybe this isn't a real problem.

As designed, it's for D-Bus, so there's not much I can suggest here,
this isn't a "generic IPC" :)

The binder developers at Samsung have stated that the implementation we
have here works for their model as well, so I guess that is some kind of
verification it's not entirely tied to D-Bus.  They have plans on
dropping the existing binder kernel code and using the kdbus code
instead when it is merged.

>  - Someone should probably convince someone who understands memory
> accounting that the pool mechanism accounts memory acceptably.  I
> don't know much about mm stuff, but I think it's subject to all kinds
> of nasty latency and accounting abuses, some of which might even be
> exploited by accident.

Michal and David agree that this all works properly.  I don't know of
anyone else to ask about it, do you?

> I haven't reviewed most of it.  I've reviewed the metadata code (and
> not recently) and the pool *docs*.
> 
> Shouldn't the bulk of this code have actual review before it gets
> merged?  I've only reviewed some of it, and I didn't like what I found
> in that small fraction, hence my objections to caps.

I'd love more review, and we have been asking for it since last October.
You provided a lot of it a while ago, and that helped immensely.

I can't force anyone to read the code, I can only go on what people
offer to do.  We have 3 signed-off-bys on the main kdbus patches, and
numerous other different developers have provided fixes / tweaks that
are in this tree, so it's not like this is unread/unposted code here at
all.

thanks,

greg k-h
--
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