Sdbusplus-based Shared Library

Sdbusplus-based Shared Library

[Patrick Venture]

We have a lot of duplication across daemons using sdbusplus, and no
new utility library.  To avoid every daemon having their own timer
object, and their own this or that, I suggest we create this shared
library.

Thoughts?  We can start simple, just have a timer object in there, and
then the new timers being introduced to phosphor-hwmon and
phosphor-host-ipmid can be the first customers.

Patrick

RE: Sdbusplus-based Shared Library

[Tanous, Ed]

Have you tried prototyping to see how much space you'll save?  I suspect it won't be a lot for a few reasons.

1. A lot of sdbusplus is template instantiations, and unless we forward declare a number of base template instantiations for use in the shared library, most of the application code will end up in the binary anyway.
2. Sdbus calls into libsystemd, which is already a shared library.
3. The filesystem is already compressed, so I suspect that any duplicated methods that aren't inlined will have the same binary code pattern and get duplicated by the squashfs filesystem.

Those are all the reasons why I haven't really looked into it for the dbus stuff;  I can't speak to the timer stuff, but I suspect the wins in size there are going to be small.

Normal disclaimer, on this specific library, I haven't prototyped anything, just done back of the napkin guesses, so I could very easily be missing something here.

-Ed

> -----Original Message-----
> From: openbmc [mailto:openbmc-
> bounces+ed.tanous=intel.com@lists.ozlabs.org] On Behalf Of Patrick
> Venture
> Sent: Monday, March 19, 2018 10:12 AM
> To: Ratan K Gupta <ratagupt@in.ibm.com>; Brad Bishop
> <bradleyb@fuzziesquirrel.com>; OpenBMC Maillist
> <openbmc@lists.ozlabs.org>
> Subject: Sdbusplus-based Shared Library
> 
> We have a lot of duplication across daemons using sdbusplus, and no new
> utility library.  To avoid every daemon having their own timer object, and
> their own this or that, I suggest we create this shared library.
> 
> Thoughts?  We can start simple, just have a timer object in there, and then
> the new timers being introduced to phosphor-hwmon and phosphor-host-
> ipmid can be the first customers.
> 
> Patrick

Re: Sdbusplus-based Shared Library

[Patrick Venture]

On Mon, Mar 19, 2018 at 11:24 AM, Tanous, Ed <ed.tanous@intel.com> wrote:
> Have you tried prototyping to see how much space you'll save?  I suspect it won't be a lot for a few reasons.
>
> 1. A lot of sdbusplus is template instantiations, and unless we forward declare a number of base template instantiations for use in the shared library, most of the application code will end up in the binary anyway.
> 2. Sdbus calls into libsystemd, which is already a shared library.
> 3. The filesystem is already compressed, so I suspect that any duplicated methods that aren't inlined will have the same binary code pattern and get duplicated by the squashfs filesystem.
>
> Those are all the reasons why I haven't really looked into it for the dbus stuff;  I can't speak to the timer stuff, but I suspect the wins in size there are going to be small.
>
> Normal disclaimer, on this specific library, I haven't prototyped anything, just done back of the napkin guesses, so I could very easily be missing something here.
>

So my thinking wasn't about reducing size of the binary but rather
reducing the toil of maintaining multiple implementations of the same
code.

> -Ed
>
>> -----Original Message-----
>> From: openbmc [mailto:openbmc-
>> bounces+ed.tanous=intel.com@lists.ozlabs.org] On Behalf Of Patrick
>> Venture
>> Sent: Monday, March 19, 2018 10:12 AM
>> To: Ratan K Gupta <ratagupt@in.ibm.com>; Brad Bishop
>> <bradleyb@fuzziesquirrel.com>; OpenBMC Maillist
>> <openbmc@lists.ozlabs.org>
>> Subject: Sdbusplus-based Shared Library
>>
>> We have a lot of duplication across daemons using sdbusplus, and no new
>> utility library.  To avoid every daemon having their own timer object, and
>> their own this or that, I suggest we create this shared library.
>>
>> Thoughts?  We can start simple, just have a timer object in there, and then
>> the new timers being introduced to phosphor-hwmon and phosphor-host-
>> ipmid can be the first customers.
>>
>> Patrick

Re: Sdbusplus-based Shared Library

[Lei YU]

On Tue, Mar 20, 2018 at 3:33 AM, Patrick Venture <venture@google.com> wrote:
> On Mon, Mar 19, 2018 at 11:24 AM, Tanous, Ed <ed.tanous@intel.com> wrote:
>> Have you tried prototyping to see how much space you'll save?  I suspect it won't be a lot for a few reasons.
>>
>> 1. A lot of sdbusplus is template instantiations, and unless we forward declare a number of base template instantiations for use in the shared library, most of the application code will end up in the binary anyway.
>> 2. Sdbus calls into libsystemd, which is already a shared library.
>> 3. The filesystem is already compressed, so I suspect that any duplicated methods that aren't inlined will have the same binary code pattern and get duplicated by the squashfs filesystem.
>>
>> Those are all the reasons why I haven't really looked into it for the dbus stuff;  I can't speak to the timer stuff, but I suspect the wins in size there are going to be small.
>>
>> Normal disclaimer, on this specific library, I haven't prototyped anything, just done back of the napkin guesses, so I could very easily be missing something here.
>>
>
> So my thinking wasn't about reducing size of the binary but rather
> reducing the toil of maintaining multiple implementations of the same
> code.
>

I am glad this is proposed again :)
I definitely support a shared library for common sdbusplus code.
I know there are comments that phosphos-xxx repos are expected to be
independent (and thus only links to sdbusplus and phosphor-dbus-interfaces)
But it does make each repo to have its own utils with common code like
"getService()", "getProperty()", and timers as well if it uses timers.

>> -Ed
>>
>>> -----Original Message-----
>>> From: openbmc [mailto:openbmc-
>>> bounces+ed.tanous=intel.com@lists.ozlabs.org] On Behalf Of Patrick
>>> Venture
>>> Sent: Monday, March 19, 2018 10:12 AM
>>> To: Ratan K Gupta <ratagupt@in.ibm.com>; Brad Bishop
>>> <bradleyb@fuzziesquirrel.com>; OpenBMC Maillist
>>> <openbmc@lists.ozlabs.org>
>>> Subject: Sdbusplus-based Shared Library
>>>
>>> We have a lot of duplication across daemons using sdbusplus, and no new
>>> utility library.  To avoid every daemon having their own timer object, and
>>> their own this or that, I suggest we create this shared library.
>>>
>>> Thoughts?  We can start simple, just have a timer object in there, and then
>>> the new timers being introduced to phosphor-hwmon and phosphor-host-
>>> ipmid can be the first customers.
>>>
>>> Patrick

RE: Sdbusplus-based Shared Library

[Tanous, Ed]

> 
> So my thinking wasn't about reducing size of the binary but rather reducing
> the toil of maintaining multiple implementations of the same code.
> 
Ah.  I totally missed the intent.   I could see it adding value if it was just to avoid code duplication in daemons.

-Ed

Re: Sdbusplus-based Shared Library

[Deepak Kodihalli]

On 19/03/18 10:41 pm, Patrick Venture wrote:
> We have a lot of duplication across daemons using sdbusplus, and no
> new utility library.  To avoid every daemon having their own timer
> object, and their own this or that, I suggest we create this shared
> library.
> 
> Thoughts?  We can start simple, just have a timer object in there, and
> then the new timers being introduced to phosphor-hwmon and
> phosphor-host-ipmid can be the first customers.
> 
> Patrick

I think sdbusplus is specifically a systemd D-Bus API wrapper. So I 
wonder if need more such wrappers (for eg one around the systemd event 
loop, to add objects like timers, IO etc) around systemd-based API than 
having them all in sdbusplus.

Regards,
Deepak

Re: Sdbusplus-based Shared Library

[Andrew Jeffery]

On Tue, 20 Mar 2018, at 06:03, Patrick Venture wrote:
> On Mon, Mar 19, 2018 at 11:24 AM, Tanous, Ed <ed.tanous@intel.com> wrote:
> > Have you tried prototyping to see how much space you'll save?  I suspect it won't be a lot for a few reasons.
> >
> > 1. A lot of sdbusplus is template instantiations, and unless we forward declare a number of base template instantiations for use in the shared library, most of the application code will end up in the binary anyway.
> > 2. Sdbus calls into libsystemd, which is already a shared library.
> > 3. The filesystem is already compressed, so I suspect that any duplicated methods that aren't inlined will have the same binary code pattern and get duplicated by the squashfs filesystem.
> >
> > Those are all the reasons why I haven't really looked into it for the dbus stuff;  I can't speak to the timer stuff, but I suspect the wins in size there are going to be small.
> >
> > Normal disclaimer, on this specific library, I haven't prototyped anything, just done back of the napkin guesses, so I could very easily be missing something here.
> >
> 
> So my thinking wasn't about reducing size of the binary but rather
> reducing the toil of maintaining multiple implementations of the same
> code.
> 

I'm on board with this idea. There are a bunch of utility classes defined for RAII that are (were) used in phosphor-mboxd that would be much better off defined in some library where they can be reused.

Has anyone looked at this beyond what's in the thread?

Cheers,

Andrew

Re: Sdbusplus-based Shared Library

[Ratan Gupta]

Hi Andrew,

We raised this concern around an year back with patrick but that time it 
was told we have limited set of users for such classes,

Let it be replicate but now as the code size is getting increased we 
should look for some common library for such classes.

Regards

Ratan Gupta


On Tuesday 27 March 2018 11:17 AM, Andrew Jeffery wrote:
> On Tue, 20 Mar 2018, at 06:03, Patrick Venture wrote:
>> On Mon, Mar 19, 2018 at 11:24 AM, Tanous, Ed <ed.tanous@intel.com> wrote:
>>> Have you tried prototyping to see how much space you'll save?  I suspect it won't be a lot for a few reasons.
>>>
>>> 1. A lot of sdbusplus is template instantiations, and unless we forward declare a number of base template instantiations for use in the shared library, most of the application code will end up in the binary anyway.
>>> 2. Sdbus calls into libsystemd, which is already a shared library.
>>> 3. The filesystem is already compressed, so I suspect that any duplicated methods that aren't inlined will have the same binary code pattern and get duplicated by the squashfs filesystem.
>>>
>>> Those are all the reasons why I haven't really looked into it for the dbus stuff;  I can't speak to the timer stuff, but I suspect the wins in size there are going to be small.
>>>
>>> Normal disclaimer, on this specific library, I haven't prototyped anything, just done back of the napkin guesses, so I could very easily be missing something here.
>>>
>> So my thinking wasn't about reducing size of the binary but rather
>> reducing the toil of maintaining multiple implementations of the same
>> code.
>>
> I'm on board with this idea. There are a bunch of utility classes defined for RAII that are (were) used in phosphor-mboxd that would be much better off defined in some library where they can be reused.
>
> Has anyone looked at this beyond what's in the thread?
>
> Cheers,
>
> Andrew
>

Re: Sdbusplus-based Shared Library

[Patrick Venture]

Brad,

If you create a repository - phosphor-sdbus-utils or
phosphor-sdbusplus-utils or something like that, I'll put together a
starter pack on this and submit for review.

Patrick

On Mon, Mar 26, 2018 at 11:04 PM, Ratan Gupta
<ratagupt@linux.vnet.ibm.com> wrote:
> Hi Andrew,
>
> We raised this concern around an year back with patrick but that time it was
> told we have limited set of users for such classes,
>
> Let it be replicate but now as the code size is getting increased we should
> look for some common library for such classes.
>
> Regards
>
> Ratan Gupta
>
>
>
> On Tuesday 27 March 2018 11:17 AM, Andrew Jeffery wrote:
>>
>> On Tue, 20 Mar 2018, at 06:03, Patrick Venture wrote:
>>>
>>> On Mon, Mar 19, 2018 at 11:24 AM, Tanous, Ed <ed.tanous@intel.com> wrote:
>>>>
>>>> Have you tried prototyping to see how much space you'll save?  I suspect
>>>> it won't be a lot for a few reasons.
>>>>
>>>> 1. A lot of sdbusplus is template instantiations, and unless we forward
>>>> declare a number of base template instantiations for use in the shared
>>>> library, most of the application code will end up in the binary anyway.
>>>> 2. Sdbus calls into libsystemd, which is already a shared library.
>>>> 3. The filesystem is already compressed, so I suspect that any
>>>> duplicated methods that aren't inlined will have the same binary code
>>>> pattern and get duplicated by the squashfs filesystem.
>>>>
>>>> Those are all the reasons why I haven't really looked into it for the
>>>> dbus stuff;  I can't speak to the timer stuff, but I suspect the wins in
>>>> size there are going to be small.
>>>>
>>>> Normal disclaimer, on this specific library, I haven't prototyped
>>>> anything, just done back of the napkin guesses, so I could very easily be
>>>> missing something here.
>>>>
>>> So my thinking wasn't about reducing size of the binary but rather
>>> reducing the toil of maintaining multiple implementations of the same
>>> code.
>>>
>> I'm on board with this idea. There are a bunch of utility classes defined
>> for RAII that are (were) used in phosphor-mboxd that would be much better
>> off defined in some library where they can be reused.
>>
>> Has anyone looked at this beyond what's in the thread?
>>
>> Cheers,
>>
>> Andrew
>>
>

Re: Sdbusplus-based Shared Library

[Brad Bishop]

> On Mar 27, 2018, at 11:43 AM, Patrick Venture <venture@google.com> wrote:
> 
> Brad,
> 
> If you create a repository - phosphor-sdbus-utils or
> phosphor-sdbusplus-utils or something like that, I'll put together a
> starter pack on this and submit for review.

May I suggest

https://github.com/openbmc/phosphor-dbus-monitor/tree/master/src/sdevent

as a possibility for drawing ideas?  :-)

> 
> Patrick

Thanks Patrick! will do but I’d like to get a bit more clarity/discussion
on what the content of this new repo will be.

So today we have sdbusplus built around the sd-bus folder in libsystemd.

Do we envision something like separate repos for:

sdeventplus - wrappers for the sd-event folder in libsystemd
sddeviceplus - wrappers for the sd-device folder in libsystemd
…etc

Or should we just try and have a single c++ wrapper repository for all of
libsystemd?  In that case should we simply rename sdbusplus to libsystemdpp
and start putting more code in it?

-brad

Re: Sdbusplus-based Shared Library

[Patrick Venture]

On Wed, Mar 28, 2018 at 6:39 PM, Brad Bishop
<bradleyb@fuzziesquirrel.com> wrote:
>
>> On Mar 27, 2018, at 11:43 AM, Patrick Venture <venture@google.com> wrote:
>>
>> Brad,
>>
>> If you create a repository - phosphor-sdbus-utils or
>> phosphor-sdbusplus-utils or something like that, I'll put together a
>> starter pack on this and submit for review.
>
> May I suggest
>
> https://github.com/openbmc/phosphor-dbus-monitor/tree/master/src/sdevent
>
> as a possibility for drawing ideas?  :-)
>
>>
>> Patrick
>
> Thanks Patrick! will do but I’d like to get a bit more clarity/discussion
> on what the content of this new repo will be.
>
> So today we have sdbusplus built around the sd-bus folder in libsystemd.
>
> Do we envision something like separate repos for:
>
> sdeventplus - wrappers for the sd-event folder in libsystemd
> sddeviceplus - wrappers for the sd-device folder in libsystemd
> …etc
>
> Or should we just try and have a single c++ wrapper repository for all of
> libsystemd?  In that case should we simply rename sdbusplus to libsystemdpp
> and start putting more code in it?

I had envisioned all the sd-bus wrappers in one library, however, I'm
quite flexible about any of that.  My primary goal with pushing this
line of thought it to reduce code duplication in openbmc.  So many
daemons have the same methods that just talk to sdbusplus, etc.  So it
makes sense to me to have a utility library for interfacing with
sdbusplus for common tasks.  phosphor::util::getService(),
phosphor::util::get... etc, so they can be maintained in one place.
Even phosphor::util::network::... because those are becoming
duplicated.  It might make sense to have multiple libraries for the
utilities, which is also fine.

>
> -brad

Re: Sdbusplus-based Shared Library

[Brad Bishop]

> On Mar 28, 2018, at 11:25 PM, Patrick Venture <venture@google.com> wrote:
> 
> On Wed, Mar 28, 2018 at 6:39 PM, Brad Bishop
> <bradleyb@fuzziesquirrel.com> wrote:
>> 
>>> On Mar 27, 2018, at 11:43 AM, Patrick Venture <venture@google.com> wrote:
>>> 
>>> Brad,
>>> 
>>> If you create a repository - phosphor-sdbus-utils or
>>> phosphor-sdbusplus-utils or something like that, I'll put together a
>>> starter pack on this and submit for review.
>> 
>> May I suggest
>> 
>> https://github.com/openbmc/phosphor-dbus-monitor/tree/master/src/sdevent
>> 
>> as a possibility for drawing ideas?  :-)
>> 
>>> 
>>> Patrick
>> 
>> Thanks Patrick! will do but I’d like to get a bit more clarity/discussion
>> on what the content of this new repo will be.
>> 
>> So today we have sdbusplus built around the sd-bus folder in libsystemd.
>> 
>> Do we envision something like separate repos for:
>> 
>> sdeventplus - wrappers for the sd-event folder in libsystemd
>> sddeviceplus - wrappers for the sd-device folder in libsystemd
>> …etc
>> 
>> Or should we just try and have a single c++ wrapper repository for all of
>> libsystemd?  In that case should we simply rename sdbusplus to libsystemdpp
>> and start putting more code in it?
> 
> I had envisioned all the sd-bus wrappers in one library, however, I'm
> quite flexible about any of that.  

I think sdbusplus has been forked for non-openbmc use so it probably
doesn’t make sense to add OpenBMC-isms to it, like the mapper for instance.
Do you agree?

I thought you were talking about extending the concept that was applied
to sdbus (thin raii wrappers around sdbus) to sdevent.  I definitely think
we need that.

I think C++ bindings around sdevent could also have general utility to someone
outside of OpenBMC so I’d again vote for not including OpenBMC-isms in a
library for something like that.

> My primary goal with pushing this
> line of thought it to reduce code duplication in openbmc.  So many
> daemons have the same methods that just talk to sdbusplus, etc.  So it
> makes sense to me to have a utility library for interfacing with
> sdbusplus for common tasks.  phosphor::util::getService(),
> phosphor::util::get... etc, so they can be maintained in one place.

I feel like a number of these are mapper related.  Could we add some
client bindings for the mapper that help here and put them in the
mapper repository?  Here is a possible example of how that could
look:

https://github.com/openbmc/phosphor-dbus-monitor/blob/master/src/sdbusplus.hpp

I wrote this class so I could mock sdbusplus calls.

> Even phosphor::util::network::... because those are becoming
> duplicated.  It might make sense to have multiple libraries for the
> utilities, which is also fine.

I guess I’m advocating for distributed utilities, aligned functionally.  I worry
about a monolithic library…it seems like it could easily become a free-for-all.

Thoughts?

> 
>> 
>> -brad

Re: Sdbusplus-based Shared Library

[Lei YU]

> I thought you were talking about extending the concept that was applied
> to sdbus (thin raii wrappers around sdbus) to sdevent.  I definitely think
> we need that.
>
> I think C++ bindings around sdevent could also have general utility to someone
> outside of OpenBMC so I’d again vote for not including OpenBMC-isms in a
> library for something like that.
>
> > My primary goal with pushing this
> > line of thought it to reduce code duplication in openbmc.  So many
> > daemons have the same methods that just talk to sdbusplus, etc.  So it
> > makes sense to me to have a utility library for interfacing with
> > sdbusplus for common tasks.  phosphor::util::getService(),
> > phosphor::util::get... etc, so they can be maintained in one place.
>
> I feel like a number of these are mapper related.  Could we add some
> client bindings for the mapper that help here and put them in the
> mapper repository?  Here is a possible example of how that could
> look:
>
> https://github.com/openbmc/phosphor-dbus-monitor/blob/master/src/sdbusplus.hpp
>

I would like to bring this topic *again*, that a shared util library is
*really* needed, either in a new repo, or in some existing repo.
Because we see so many service are implementing the same functions like
"getService" "getProperty", etc, which is so common, that it's preferred to
have a common library provide the implementation, as well as the mock
interfaces.

Then every service using such functions could easily call the library, and the
tests could easily use the mocked interface.

Could we get some progress on this?

Thanks!

Re: Sdbusplus-based Shared Library

[vishwa]

Utility library for timers / watchdog was something that I initially 
proposed during the early days and the argument back was that when we 
have such a thing, then we need to keep augmenting the library to suit 
needs of the consumers and we loose flexibility and end up putting hacks.

!! Vishwa !!

On 03/19/2018 10:41 PM, Patrick Venture wrote:
> We have a lot of duplication across daemons using sdbusplus, and no
> new utility library.  To avoid every daemon having their own timer
> object, and their own this or that, I suggest we create this shared
> library.
>
> Thoughts?  We can start simple, just have a timer object in there, and
> then the new timers being introduced to phosphor-hwmon and
> phosphor-host-ipmid can be the first customers.
>
> Patrick
>

Re: Sdbusplus-based Shared Library

[Thomaiyar, Richard Marian]

I second Patrick here. We definitely need miscellaneous utility library. 
As he mentioned we can start small or even create few libraries with 
different purpose each (say timer, sdbusplus utils shared library etc.)

Related to customization, we need to make sure libraries are generic in 
nature, and expose correct hooks. Even if specific hack is needed, then 
it has to be extended as sub-module and patched by direct linking for 
the needed module instead of updating the libraries directly. ??

Regards,

Richard


On 7/18/2018 5:04 PM, vishwa wrote:
> lity library for timers / watchdog was something that I initially 
> proposed during the early days and the argument back was that when we 
> have such a thing, then we need to keep augmenting the library to suit 
> needs of the consumers and we loose flexibility and end up putting hacks.