pthreads at bmcweb

pthreads at bmcweb

[Sunitha Harish]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

Hi team,

Reference commit 
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/31735 :

In order to handle the multiple push-style event subscribers, bmc needs 
to support the async resolution of the subscribers address. The 
async_resolve() API crashes if there is no thread support in the binary.

I created a bmcweb binary patch by pulling this commit and including the 
pthread. This works fine for the use-cases, but increased the bmcweb 
binary size by 220KB.

Ed's suggestion is not to use the pthreads, instead implement 
alternatives to do the same job, so that the binary size is kept 
minimum. He mentioned: /"//Considering that's a ~30% increase in binary 
size to support one line off code, and most systems are already at their 
binary size limit, no, that's not going to be acceptable. We can either 
patch boost to use this 
//https://man7.org/linux/man-pages/man3/getaddrinfo_a.3.html 
<https://man7.org/linux/man-pages/man3/getaddrinfo_a.3.html>//or we 
could build our own resolver type that calls that underneath. This was 
based on a quick lookthrough of solutions in Google. I'm open to other 
ideas here". /

I am looking for the community views about the increased bmcweb binary 
size v/s having a custom implementation for asyc_resolve. Please share 
your views & ideas to get to the best solution.


Thanks & regards,
Sunitha



[-- Attachment #2: Type: text/html, Size: 4424 bytes --]

Re: pthreads at bmcweb

[Milton Miller II]

On Jan 6, 2021 Sunitha Harish wrote:
>     Hi team,
>     Reference commit
>https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/31735 :
>     In order to handle the multiple push-style event subscribers,
>bmc       needs to support the async resolution of the subscribers
>address.       The async_resolve() API crashes if there is no thread
>support in       the binary. 
>     I created a bmcweb binary patch by pulling this commit and
>including the pthread. This works fine for the use-cases, but
>increased the bmcweb binary size by 220KB. 
?
>     Ed's suggestion is not to use the pthreads, instead implement
> alternatives to do the same job, so that the binary size is kept
>minimum. He mentioned: "Considering that's a ~30% increase in binary
>size to support one line off code, and most systems are already at
>their binary size limit, no, that's not going to be acceptable. We
>can either patch boost to use this
>https://man7.org/linux/man-pages/man3/getaddrinfo_a.3.html or we
>could build our own resolver type that calls that underneath.  This
>was based on a quick lookthrough of solutions in Google.  I'm open to
>other ideas here". 
>     I am looking for the community views about the increased bmcweb
>     binary size v/s having a custom implementation for asyc_resolve.
>      Please share your views & ideas to get to the best solution.

I agree with Ed that adding pthreads is a step that should be taken 
with a lot of caution.   In addition to the binary size, a threaded 
application also adds security audit concerns.

A quick search with the query of "dns lookup library embedded" found 
a possible library with a probably compatible license (although last 
update was 4 years ago), and the second link was a survey of other 
client and server packages that could be investigated.

My personal opinion only.

milton

Re: pthreads at bmcweb

[Sunitha Harish]

Thanks Milton for sharing your views.

Awaiting more inputs/feedbacks from the community.

Regards,
Sunitha

On 06-01-2021 23:42, Milton Miller II wrote:
> On Jan 6, 2021 Sunitha Harish wrote:
>>      Hi team,
>>      Reference commit
>> https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/31735 :
>>      In order to handle the multiple push-style event subscribers,
>> bmc       needs to support the async resolution of the subscribers
>> address.       The async_resolve() API crashes if there is no thread
>> support in       the binary.
>>      I created a bmcweb binary patch by pulling this commit and
>> including the pthread. This works fine for the use-cases, but
>> increased the bmcweb binary size by 220KB.
> ?
>>      Ed's suggestion is not to use the pthreads, instead implement
>> alternatives to do the same job, so that the binary size is kept
>> minimum. He mentioned: "Considering that's a ~30% increase in binary
>> size to support one line off code, and most systems are already at
>> their binary size limit, no, that's not going to be acceptable. We
>> can either patch boost to use this
>> https://man7.org/linux/man-pages/man3/getaddrinfo_a.3.html or we
>> could build our own resolver type that calls that underneath.  This
>> was based on a quick lookthrough of solutions in Google.  I'm open to
>> other ideas here".
>>      I am looking for the community views about the increased bmcweb
>>      binary size v/s having a custom implementation for asyc_resolve.
>>       Please share your views & ideas to get to the best solution.
> I agree with Ed that adding pthreads is a step that should be taken
> with a lot of caution.   In addition to the binary size, a threaded
> application also adds security audit concerns.
>
> A quick search with the query of "dns lookup library embedded" found
> a possible library with a probably compatible license (although last
> update was 4 years ago), and the second link was a survey of other
> client and server packages that could be investigated.
>
> My personal opinion only.
>
> milton
>

Re: pthreads at bmcweb

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

On Wed, Jan 06, 2021 at 01:32:11PM +0530, Sunitha Harish wrote:
> Ed's suggestion is not to use the pthreads, instead implement 
> alternatives to do the same job, so that the binary size is kept 
> minimum. He mentioned: /"//Considering that's a ~30% increase in binary 
> size to support one line off code, and most systems are already at their 
> binary size limit, no, that's not going to be acceptable. We can either 
> patch boost to use this 
> //https://man7.org/linux/man-pages/man3/getaddrinfo_a.3.html 
> <https://man7.org/linux/man-pages/man3/getaddrinfo_a.3.html>//or we 
> could build our own resolver type that calls that underneath. This was 
> based on a quick lookthrough of solutions in Google. I'm open to other 
> ideas here". /

Since we're using systemd and we have the `resolved` feature turned on,
why not just send an async dbus message to the resolved.service?

```
$ busctl introspect org.freedesktop.resolve1 /org/freedesktop/resolve1 | grep ResolveHostname
.ResolveHostname                    method    isit          a(iiay)st                                -
```

https://www.freedesktop.org/software/systemd/man/org.freedesktop.resolve1.html

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: pthreads at bmcweb

[Sunitha Harish]

On 12-01-2021 18:35, Patrick Williams wrote:
> On Wed, Jan 06, 2021 at 01:32:11PM +0530, Sunitha Harish wrote:
>> Ed's suggestion is not to use the pthreads, instead implement
>> alternatives to do the same job, so that the binary size is kept
>> minimum. He mentioned: /"//Considering that's a ~30% increase in binary
>> size to support one line off code, and most systems are already at their
>> binary size limit, no, that's not going to be acceptable. We can either
>> patch boost to use this
>> //https://man7.org/linux/man-pages/man3/getaddrinfo_a.3.html
>> <https://man7.org/linux/man-pages/man3/getaddrinfo_a.3.html>//or we
>> could build our own resolver type that calls that underneath. This was
>> based on a quick lookthrough of solutions in Google. I'm open to other
>> ideas here". /
> Since we're using systemd and we have the `resolved` feature turned on,
> why not just send an async dbus message to the resolved.service?
>
> ```
> $ busctl introspect org.freedesktop.resolve1 /org/freedesktop/resolve1 | grep ResolveHostname
> .ResolveHostname                    method    isit          a(iiay)st                                -
> ```
>
> https://www.freedesktop.org/software/systemd/man/org.freedesktop.resolve1.html

Thank you Patrick. I will try implementing this and verify if all the 
async event notification scenarios works good.

Regards,
Sunitha

Re: pthreads at bmcweb

[Ed Tanous]

Another option might be something like c-ares, the description of which is:

c-ares... is intended for applications which need to perform DNS
queries without blocking, or need to perform multiple DNS queries in
parallel. The primary examples of such applications are servers which
communicate with multiple clients and programs with graphical user
interfaces.


Sounds like exactly our problem statement, so it might be worth a look
to see how much binary size it adds.

On Wed, Jan 6, 2021 at 12:02 AM Sunitha Harish
<sunithaharish04@gmail.com> wrote:
>
> Hi team,
>
> Reference commit https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/31735 :
>
> In order to handle the multiple push-style event subscribers, bmc needs to support the async resolution of the subscribers address. The async_resolve() API crashes if there is no thread support in the binary.
>
> I created a bmcweb binary patch by pulling this commit and including the pthread. This works fine for the use-cases, but increased the bmcweb binary size by 220KB.
>
> Ed's suggestion is not to use the pthreads, instead implement alternatives to do the same job, so that the binary size is kept minimum. He mentioned: "Considering that's a ~30% increase in binary size to support one line off code, and most systems are already at their binary size limit, no, that's not going to be acceptable. We can either patch boost to use this https://man7.org/linux/man-pages/man3/getaddrinfo_a.3.html or we could build our own resolver type that calls that underneath. This was based on a quick lookthrough of solutions in Google. I'm open to other ideas here".
>
> I am looking for the community views about the increased bmcweb binary size v/s having a custom implementation for asyc_resolve. Please share your views & ideas to get to the best solution.
>
>
> Thanks & regards,
> Sunitha
>
>