fork1(): Remove an obsolete comment

[olce]

Summary

This comment was introduced in commit "Add racct. (...)" (SVN revision
220137, git ID 097055e26d9459c7). It refers to missing bumping resource
counters at fork, but this is not necessary (and would actually be
harmful).

Process' credentials' resources are tracked via the 'struct ucred'
pointed to by 'p_ucred' in its 'cr_uidinfo' and 'cr_ruidinfo' fields (of
type 'struct uidinfo') and, on RACCT, through various 'struct racct' or
'struct prison_racct' structures pointed to by 'ui_racct' (for
user-scope limits), 'pr_prison_racct' (for jail-scope limits) and
'lc_racct' (for login-class-scope limits).

The fields of 'struct uidinfo' only concern resources that are either
shared or unshared at fork, but never copied, so there is no need to
update them. Additionally, the RACCT-related parts are already dealt
with by racct_proc_fork().

Fixes: 097055e26d94 Add racct. (...)
MFC after: 1 week
Sponsored by: The FreeBSD Foundation

[emaste]

In the commit please include a Fixes: 12cec311e607 ("fork: assign refed credentials earlier")

[kib]

I do not think that the comment about copying ucred without bumping refcounter. It states that _resource_ counters should be bumped, not ref count. And now I think that this bump could be missed there, after the refactoring.

[olce]

This comment was introduced in commit 097055e26d9459c7def70856aab62b77d627511d ("Add racct. (...)"). You're probably right.

But then I think it was obsolete from the start, because racct_proc_fork() seems to be doing the bumps it talks about (the block with comment "Inherit resource usage."). Am I missing something?

[olce]

You may be referring to cr_uidinfo and cr_ruidinfo on struct ucred. If the refactoring you're talking about is the fork1() one, on a cursory look I didn't find any change in behavior. The different fields of struct uidinfo are decremented only when the associated reference-counted objects are destroyed, and I suspect that the just-created process simply adds references to the latter (except perhaps for unshared parts if using rfork()), in which case the fields don't need adjustment (shared resources are already counted). Moreover, if there was a problem in accounting here, we would see messages from uifree() and chglimit() (more checks could be added). So there's very little chance that this comment is accurate: There might be problems in corner cases, but there doesn't seem to be such a general issue.

[olce]

Change the justification for removing this comment.

[kib]

My attempt to interpret this comment is following: we copy resource usage data early, which makes any resource accounting occuring from this point, not be reflected in the child resource counters. This makes child' resource accounting not accurate.

[olce]

In D42254#964848, @kib wrote:

My attempt to interpret this comment is following: we copy resource usage data early, which makes any resource accounting occuring from this point, not be reflected in the child resource counters. This makes child' resource accounting not accurate.

My understanding, as stated in part in the new commit message above, is that:

1. In fact there is no resource usage copying at all via p_ucred, only sharing of it (via sharing of p_ucred). Nothing here is in fact per-process, and it doesn't matter in this case when the pointed fields are updated with respect to setting p_ucred in the new process.
2. The RACCT portions are already handled, notably the per-proc resource counters (the proposed commit message needs an update, since it doesn't mention the p_racct field as pointing to a struct racct, but apart from that I still think it is correct).

Point 2. is also not related to credentials in any way, except via p_ucred and then cr_uidinfo and cr_ruidinfo, both via their ui_racct field, but again this structure is shared accross the parent and child (via p_ucred again).

So based on that understanding, it's fair to say that not only this comment is misplaced, but it is simply wrong because "per-cred" counters must not be updated.

But let's go even further.

The only resource accounting I've not talked about above is done in p_rux and p_crux (and p_ru), which are not related to p_ucred at all. These process' fields of type struct rusage_ext are reset to 0 at fork, and by the very nature of what they measure (run time and ticks), I don't see why they should not. Although at first less obvious, I think it's the same for fields in struct rusage.

The same reasoning can be applied to td_ru and td_rux on struct thread.

So I'm failing to see how the child counters could not be accurate in *any* case (not only the "per-cred" counters). Am I still missing something? Are there flaws in my reasoning?