core/service: try to query for new main process's starttime

[YHNdnzj]

Currently, when service_set_main_pidref() is called without specifying start_timestamp, exec_status_start() always uses dual_timestamp_now(). This is not ideal, though, as when the main pid changes halfway due to e.g. sd_notify + MAINPID=, it's definitely spurious.

[github-actions]

Important

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

[bluca]

Is this due to recent changes? Or is it something that was always there?

[YHNdnzj]

Is this due to recent changes? Or is it something that was always there?

I think this was never done correctly, so it's OK to target v257.

[bluca]

Sounds like a bug though, and it's for metrics purpose only, so should be fine to merge already

[poettering]

so i presume the process start_time is actually the fork() time, right, and not updated at execve() time, right?

assuming that this is the case, code looks roughly ok, but the clock confusion needs to be fixed.

[YHNdnzj]

so i presume the process start_time is actually the fork() time, right, and not updated at execve() time, right?

According to this piece of comment, yes.

(I didn't check yesterday, partly because I was tired, but also because I think exec() time is still better than now().)

[poettering]

so i presume the process start_time is actually the fork() time, right, and not updated at execve() time, right?

According to this piece of comment, yes.

(I didn't check yesterday, partly because I was tired, but also because I think exec() time is still better than now().)

well, if it was the exec() time, then it should probably be placed in the "handoff" timestamp.

[poettering]

So i given the inaccuracy of jiffies I think we should take more care not to override any timestamps if we forked off the process ourselves. In particular as deriving the realtime timestamp from the boottime timestamp is a bit icky, and we should really avoid doing that if we don#t really have to.

[poettering]

Sounds like a bug though, and it's for metrics purpose only, so should be fine to merge already

dunno, i disagree, i don#t see the implications of this at all. Let's not hurry this. I am pretty sure this should be 257 material.

[YHNdnzj]

So i given the inaccuracy of jiffies I think we should take more care not to override any timestamps if we forked off the process ourselves. In particular as deriving the realtime timestamp from the boottime timestamp is a bit icky, and we should really avoid doing that if we don#t really have to.

We don't? If the main process is forked off by us, we always pass in start_timestamp (see service_enter_start)

[poettering]

So, this does redefine what the start timestamp actually means. previously it meant: timestamp when the main process become the main process. with this pr applied it means timestampt when the process that is currently the main process started. Which are two distinct definitions.

I think the latter does make some sense in that it is less confusing to users probably. hence i think we probably should make the change, but really, such as change is v257 material i am sure.

[poettering]

So i given the inaccuracy of jiffies I think we should take more care not to override any timestamps if we forked off the process ourselves. In particular as deriving the realtime timestamp from the boottime timestamp is a bit icky, and we should really avoid doing that if we don#t really have to.

We don't? If the main process is forked off by us, we always pass in start_timestamp (see service_enter_start)

ah, sorry, i misread the func, i though it would update the tstamp always, but it only does it when the pid changes. hence, indeed we should be good.

[bluca]

So i given the inaccuracy of jiffies I think we should take more care not to override any timestamps if we forked off the process ourselves. In particular as deriving the realtime timestamp from the boottime timestamp is a bit icky, and we should really avoid doing that if we don#t really have to.

We don't? If the main process is forked off by us, we always pass in start_timestamp (see service_enter_start)

ah, sorry, i misread the func, i though it would update the tstamp always, but it only does it when the pid changes. hence, indeed we should be good.

Yeah this is only in a particular case where we know it's wrong. Given it's only in that case, and it's only for display purpose (eg, our behaviour is not affected), it's fine for me to either take it immediately or postpone, don't mind either way.

[YHNdnzj]

I have no strong opinion on which release this should target, either.