openbmc / phosphor-led-manager

Apache License 2.0
5 stars 12 forks source link

Setting an "Asserted" property on xyz.openbmc_project.LED.GroupManager takes a long time #9

Open brandonkimbk opened 3 years ago

brandonkimbk commented 3 years ago

This is to bring the attention to a problem that was discovered from phosphor-nvme https://github.com/openbmc/phosphor-nvme/issues/4

I found that phosphor-nvme daemon was causing issues with the D-Bus, and realized that while the D-Bus is being stressed, it can take almost 133ms to call a setProperty to xyz.openbmc_project.LED.GroupManager and xyz.openbmc_project.Led.Group for "Asserted" property. This is in contrast to

Looking at the code for the setter, we can see that it should compare and return right away if it's the same https://github.com/openbmc/phosphor-led-manager/blob/87fd11c9f6b8bc503afa32ac54bf0ed89c851048/group.cpp#L12-L19

The mitigation for phosphor-nvme was to call the getter manually and not call the setter when the property did not have to change - this decreased the average time by 87% which was much more reasonable.

Why could this be the case? The "setter" in this case "should" just be doing what a "getter" should be doing if the property is the same, however we're seeing a huge performance drop for doing a set.

Workaround:

https://github.com/openbmc/phosphor-nvme/blob/fdffe5c37f0d1feaa90558433f688c3757d2e79a/nvme_manager.cpp#L116-L125

https://github.com/openbmc/phosphor-nvme/blob/fdffe5c37f0d1feaa90558433f688c3757d2e79a/nvme_manager.cpp#L155-L161

pointbazaar commented 2 months ago

Since it's been 3 years, with many changes to the code, is this issue still present?

williamspatrick commented 2 months ago

This seems like something that should be done in the generated sdbusplus bindings rather than sprinkled around each daemon. I'll check on that.