openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* service-config-manager performance issue
@ 2022-02-21 12:41 Jiaqing Zhao
  2022-02-21 20:00 ` Joseph Reynolds
  0 siblings, 1 reply; 4+ messages in thread
From: Jiaqing Zhao @ 2022-02-21 12:41 UTC (permalink / raw)
  To: openbmc; +Cc: apparao.puli, richard.marian.thomaiyar

Hi, all

When updating service status with service-config-manager, it always takes ~15s for the new status to be applied, which is much longer than it should be.

By inspecting the code, I found there is a 15s wait before updating service status in ServiceConfig::startServiceRestartTimer(). (https://github.com/openbmc/service-config-manager/blob/f2744893b77b9dd8903bb13113f4c3ef62c18f04/src/srvcfg_manager.cpp#L382)

The 15s-wait is added at first in commit 0084047 (https://github.com/openbmc/service-config-manager/commit/0084047d008fd0ac36f09a232f67ff2fc5314b53).

I've tested service-config-manager works as expected with the wait removed, and it only takes ~1s for the settings being applied, shall we remove it? And I'd like to ask what is the purpose of this wait to double confirm if removing it will bring any side effect.

Thanks,
Jiaqing

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: service-config-manager performance issue
  2022-02-21 12:41 service-config-manager performance issue Jiaqing Zhao
@ 2022-02-21 20:00 ` Joseph Reynolds
  2022-02-22  4:06   ` Jiaqing Zhao
  2022-03-07  9:25   ` Jiaqing Zhao
  0 siblings, 2 replies; 4+ messages in thread
From: Joseph Reynolds @ 2022-02-21 20:00 UTC (permalink / raw)
  To: Jiaqing Zhao, openbmc; +Cc: apparao.puli, richard.marian.thomaiyar

On 2/21/22 6:41 AM, Jiaqing Zhao wrote:
> Hi, all
>
> When updating service status with service-config-manager, it always takes ~15s for the new status to be applied, which is much longer than it should be.
>
> By inspecting the code, I found there is a 15s wait before updating service status in ServiceConfig::startServiceRestartTimer(). (https://github.com/openbmc/service-config-manager/blob/f2744893b77b9dd8903bb13113f4c3ef62c18f04/src/srvcfg_manager.cpp#L382)
>
> The 15s-wait is added at first in commit 0084047 (https://github.com/openbmc/service-config-manager/commit/0084047d008fd0ac36f09a232f67ff2fc5314b53).

Here at IBM we are seeing the same thing.  It looks like that code 
(https://github.com/openbmc/service-config-manager/blob/master/src/srvcfg_manager.cpp 
- ServiceConfig::startServiceRestartTimer) was part of the initial 
commit.  I wonder if the problem is caused by a bug in that code.  (But 
I am not familiar with the code, and I don't have time to look at it 
now.)  Is the intention to reload the systemd units but give up after 15 
seconds?

Joseph

> I've tested service-config-manager works as expected with the wait removed, and it only takes ~1s for the settings being applied, shall we remove it? And I'd like to ask what is the purpose of this wait to double confirm if removing it will bring any side effect.
>
> Thanks,
> Jiaqing


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: service-config-manager performance issue
  2022-02-21 20:00 ` Joseph Reynolds
@ 2022-02-22  4:06   ` Jiaqing Zhao
  2022-03-07  9:25   ` Jiaqing Zhao
  1 sibling, 0 replies; 4+ messages in thread
From: Jiaqing Zhao @ 2022-02-22  4:06 UTC (permalink / raw)
  To: Joseph Reynolds, openbmc; +Cc: apparao.puli, richard.marian.thomaiyar

On 2022-02-22 04:00, Joseph Reynolds wrote:
> On 2/21/22 6:41 AM, Jiaqing Zhao wrote:
>> Hi, all
>>
>> When updating service status with service-config-manager, it always takes ~15s for the new status to be applied, which is much longer than it should be.
>>
>> By inspecting the code, I found there is a 15s wait before updating service status in ServiceConfig::startServiceRestartTimer(). (https://github.com/openbmc/service-config-manager/blob/f2744893b77b9dd8903bb13113f4c3ef62c18f04/src/srvcfg_manager.cpp#L382)
>>
>> The 15s-wait is added at first in commit 0084047 (https://github.com/openbmc/service-config-manager/commit/0084047d008fd0ac36f09a232f67ff2fc5314b53).
> 
> Here at IBM we are seeing the same thing.  It looks like that code (https://github.com/openbmc/service-config-manager/blob/master/src/srvcfg_manager.cpp - ServiceConfig::startServiceRestartTimer) was part of the initial commit.  I wonder if the problem is caused by a bug in that code.  (But I am not familiar with the code, and I don't have time to look at it now.)  Is the intention to reload the systemd units but give up after 15 seconds?
> 
> Joseph
>

I think the author intends to execute systemd operations with a 15s timeout. But seems he misused boost::asio::steady_timer. The boost doc (https://www.boost.org/doc/libs/1_78_0/doc/html/boost_asio/reference/steady_timer.html) says the function in async_wait() is called when the timer has expired. So the code in service-config-manager actually calls the apply config function after asynchronously waited for 15s.

I cannot find out the proper a way to set a 15s time limit for the operation. Maybe we can set a max wait rounds of executing a systemd job in systemdUnitAction()? Currently it waits infinitely in ~20ms interval.

Jiaqing

>> I've tested service-config-manager works as expected with the wait removed, and it only takes ~1s for the settings being applied, shall we remove it? And I'd like to ask what is the purpose of this wait to double confirm if removing it will bring any side effect.
>>
>> Thanks,
>> Jiaqing
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: service-config-manager performance issue
  2022-02-21 20:00 ` Joseph Reynolds
  2022-02-22  4:06   ` Jiaqing Zhao
@ 2022-03-07  9:25   ` Jiaqing Zhao
  1 sibling, 0 replies; 4+ messages in thread
From: Jiaqing Zhao @ 2022-03-07  9:25 UTC (permalink / raw)
  To: Joseph Reynolds, openbmc; +Cc: apparao.puli, richard.marian.thomaiyar

Just had a discussion with the original author of that function, the 15s wait here is used to accommodate multiple config changes in single restart. If remove this wait, each service change will trigger systemd reload, which causes race condition and loss of data.

So for services controlled by service-config-manager, both get and set operations should call the manager's DBus. An example is intel-ipmi-oem (https://github.com/openbmc/intel-ipmi-oem/blob/master/src/bmccontrolservices.cpp). But in bmcweb, the get is implemented by calling systemd DBus directly,
so the properies in Redfish will not reflect the changes immediately. I will submit a patch to fix it. 

On 2022-02-22 04:00, Joseph Reynolds wrote:
> On 2/21/22 6:41 AM, Jiaqing Zhao wrote:
>> Hi, all
>>
>> When updating service status with service-config-manager, it always takes ~15s for the new status to be applied, which is much longer than it should be.
>>
>> By inspecting the code, I found there is a 15s wait before updating service status in ServiceConfig::startServiceRestartTimer(). (https://github.com/openbmc/service-config-manager/blob/f2744893b77b9dd8903bb13113f4c3ef62c18f04/src/srvcfg_manager.cpp#L382)
>>
>> The 15s-wait is added at first in commit 0084047 (https://github.com/openbmc/service-config-manager/commit/0084047d008fd0ac36f09a232f67ff2fc5314b53).
> 
> Here at IBM we are seeing the same thing.  It looks like that code (https://github.com/openbmc/service-config-manager/blob/master/src/srvcfg_manager.cpp - ServiceConfig::startServiceRestartTimer) was part of the initial commit.  I wonder if the problem is caused by a bug in that code.  (But I am not familiar with the code, and I don't have time to look at it now.)  Is the intention to reload the systemd units but give up after 15 seconds?
> 
> Joseph
> 
>> I've tested service-config-manager works as expected with the wait removed, and it only takes ~1s for the settings being applied, shall we remove it? And I'd like to ask what is the purpose of this wait to double confirm if removing it will bring any side effect.
>>
>> Thanks,
>> Jiaqing
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-03-07  9:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-21 12:41 service-config-manager performance issue Jiaqing Zhao
2022-02-21 20:00 ` Joseph Reynolds
2022-02-22  4:06   ` Jiaqing Zhao
2022-03-07  9:25   ` Jiaqing Zhao

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