core: only activate transaction that contain useful jobs#40314
core: only activate transaction that contain useful jobs#40314YHNdnzj merged 1 commit intosystemd:mainfrom
Conversation
YHNdnzj
left a comment
There was a problem hiding this comment.
Code itself makes sense to me, but I don't entirely understand the connection to the linked issue. The way I read things this only suppresses jobs from being installed if there's nothing load-bearing, but doesn't address the underlying issue?
src/core/manager.c
Outdated
|
|
||
| /* Only activate the transaction if it contains jobs other than NOP anchor. | ||
| * Short-circuiting here avoids unnecessary processing, such as emitting D-Bus signals. */ | ||
| if (hashmap_size(tr->jobs) == 1) |
There was a problem hiding this comment.
Hmmm...Isn't that confusing? I mean we want early return in exactly one case so why check interval. On top of that, we put anchor job to the transaction explicitly.
There was a problem hiding this comment.
Yeah I know == 0 is in reality not reachable, but we tend to encode the intention in these comparisons, and 0 is conceptually something we should skip too
AFAICT, the issue as described is not reproducible any more with current systemd version. I was able to reproduce 50%, i.e. NOP jobs are present after reload but eventually they are cleared because run_queue is correctly triggered after coldpluging stuff after deserialization. So the second part, i.e. these NOP jobs are stuck forever is no longer the case. |
If no real jobs were added to the transaction, do not activate it. The JOB_NOP anchor does not perform any useful work and activating such transaction only wastes resources. Fixes systemd#9751
629f082 to
e88fcd5
Compare
|
@YHNdnzj updated. PTAL. |
yuwata
left a comment
There was a problem hiding this comment.
LGTM.
Could you add a test case for the issue?
I think it is better to add a sub test in TEST-17-UDEV.
|
OK, merging this as-is now. As explained above this is just an optimization, hence doesn't strictly speaking need a test. Test cases are still nice to have, but I see no reason to block the PR for that. |
If no real jobs were added to the transaction, do not activate it. The JOB_NOP anchor does not perform any useful work and activating such transaction only wastes resources.
Fixes #9751