[systemd-devel] [PATCH 3/3] kdbus: get some creds during meta append for optimization

[Djalal Harouni]

On Tue, Aug 19, 2014 at 09:15:35AM +0200, Daniel Mack wrote:
> Hi Djalal,
Thanks for applying the others.


> On 08/19/2014 03:43 AM, Djalal Harouni wrote:
> > Some creds can be gathered during kdbus_meta_append() instead of
> > kdbus_conn_queue_alloc() where they will be gathered for all the
> > receivers and saved into each receivers queue before installing
> > on the slices.
> > 
> > By moving to kdbus_meta_append() it permits to do it only one time
> > for all the receivers, and we reduce some latency in
> > kdbus_conn_queue_alloc().
> > 
> > Another point is that all the receivers will get the same uid/gid even
> > if the current sender changes its creds while we are still queueing for
> > all receivers.
> > 
> > It seems that we can do the same with auxgroups, but this patch does
> > not implement it.
> > 
> > Patch tested with the test-kdbus-metadata-ns
> > 
> > Signed-off-by: Djalal Harouni <tixxdz at opendz.org>
> > ---
> >  connection.c | 56 ++++++++++++++++++++++++++++++++++++--------------------
> >  metadata.c   |  6 +++++-
> >  metadata.h   | 26 ++++++++++++++++++++++++++
> >  3 files changed, 67 insertions(+), 21 deletions(-)
> > 
> > diff --git a/connection.c b/connection.c
> > index 9250dab..49c5045 100644
> > --- a/connection.c
> > +++ b/connection.c
> > @@ -42,6 +42,24 @@
> >  struct kdbus_conn_reply;
> >  
> >  /**
> > + * struct kdbus_queue_creds - internal creds data for receiver's
> > + *			      queue
> > + * @uid:		The UID to patch into the final message
> > + * @gid:		The GID to patch into the final message
> > + * @pid:		The PID to patch into the final message
> > + * @tid:		The TID to patch into the final message
> > + *
> > + * Creds that must be translated into the receiver's namespaces
> > + * when the message is installed into its slice.
> > + */
> > +struct kdbus_queue_creds {
> > +	kuid_t uid;
> > +	kgid_t gid;
> > +	struct pid *pid;
> > +	struct pid *tid;
> > +};
> 
> Hmm, I'm not convinced this buys us anything really. After all, that
> struct has a single user only, and factoring out these fields doesn't
> necessarily lead to more readability.
Hmm with your commit 7bde48f293f5, I guess we have to add a generic
struct like that, or split into multiple structs, the aim would be to
share the struct and operations that perform the:

Translate and install fields into the receiver's slice when requesting 
the connection info or when the receiver reads the message ?



> [...]
> 
> >  /**
> > + * struct kdbus_meta_creds - internal creds data
> > + * @uid:                The UID of the sender
> > + * @gid:                The GID of the sender
> > + *
> > + * Creds used for optimizations. We need to gather the info during
> > + * kdbus_meta_append() before pushing into the receiver's queue,
> > + * this way we do it only one time for all the receivers.
> 
> I'm not strictly against it, but I wonder if this optimization is really
> worth it. Do you think it's that expensive to access these fields in
> 'current'? Because this change certainly doesn't make the code easier to
> understand, so I'm hesitant about this change.

Yes, I agree that the code would not be easy to read, and I failed to
come up with a better solution, perhaps it needs more thoughts!

Well, yes without numbers the optimization for uid/gid perhaps is not
needed, but for the auxgroup perhaps ? we can allocate and get the
auxgrps one time, then just memdup the data into the queue->auxgrps[] ?

Just trying to figure out the best solution...


BTW Daniel, sorry for this stupid question:
Can't the KDBUS_ATTACH_AUXGROUPS be part of the KDBUS_ATTACH_CREDS item?

Will be easy to parse it in the same item with uid/gid...


-- 
Djalal Harouni
http://opendz.org