Re: git: 32114b639fa1 - main - Add PROC_COW_CHANGECOUNT and thread_cow_synced

On 2/13/22, Konstantin Belousov <kostikbel@gmail.com> wrote:
> On Sat, Feb 12, 2022 at 10:22:28PM +0100, Mateusz Guzik wrote:
>> On 2/12/22, Konstantin Belousov <kostikbel@gmail.com> wrote:
>> > On Sat, Feb 12, 2022 at 07:50:21PM +0100, Mateusz Guzik wrote:
>> >> On 2/11/22, Konstantin Belousov <kostikbel@gmail.com> wrote:
>> >> > On Fri, Feb 11, 2022 at 01:57:59PM +0000, Mateusz Guzik wrote:
>> >> >> The branch main has been updated by mjg:
>> >> >>
>> >> >> URL:
>> >> >> https://cgit.FreeBSD.org/src/commit/?id=32114b639fa1ad777312eebe14a9f677bd7be2ea
>> >> >>
>> >> >> commit 32114b639fa1ad777312eebe14a9f677bd7be2ea
>> >> >> Author:     Mateusz Guzik <mjg@FreeBSD.org>
>> >> >> AuthorDate: 2022-02-01 13:13:13 +0000
>> >> >> Commit:     Mateusz Guzik <mjg@FreeBSD.org>
>> >> >> CommitDate: 2022-02-11 11:44:07 +0000
>> >> >>
>> >> >>     Add PROC_COW_CHANGECOUNT and thread_cow_synced
>> >> >>
>> >> >>     Combined they can be used to avoid a proc lock/unlock cycle in
>> >> >> the
>> >> >>     syscall handler for curthread, see upcoming examples.
>> >> >> ---
>> >> >>  sys/kern/kern_thread.c | 13 +++++++++++++
>> >> >>  sys/sys/proc.h         |  9 +++++++++
>> >> >>  2 files changed, 22 insertions(+)
>> >> >>
>> >> >> diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c
>> >> >> index dcb52b137b58..bb724a17803e 100644
>> >> >> --- a/sys/kern/kern_thread.c
>> >> >> +++ b/sys/kern/kern_thread.c
>> >> >> @@ -868,6 +868,19 @@ thread_cow_update(struct thread *td)
>> >> >>  		lim_free(oldlimit);
>> >> >>  }
>> >> >>
>> >> >> +void
>> >> >> +thread_cow_synced(struct thread *td)
>> >> >> +{
>> >> >> +	struct proc *p;
>> >> >> +
>> >> >> +	p = td->td_proc;
>> >> >> +	PROC_LOCK_ASSERT(p, MA_OWNED);
>> >> >> +	MPASS(td->td_cowgen != p->p_cowgen);
>> >> >> +	MPASS(td->td_ucred == p->p_ucred);
>> >> >> +	MPASS(td->td_limit == p->p_limit);
>> >> >> +	td->td_cowgen = p->p_cowgen;
>> >> > This should be store-release, I think.
>> >> > And corresponding loads in trap() needs to get acquire semantic.
>> >> >
>> >> > This is probably a pre-existing bug.
>> >>
>> >> I don't think adding fences would improve anything here. First note
>> >> fences or not, the thread can still race against cowgen changing and
>> >> miss it this time around. At the same time all updates to cowgen are
>> >> done with process lock, which will also be taken to sync. Consequently
>> >> the thread at hand in the worst case will miss cowgen being updated
>> >> and will act on it next time. If it decides to act on cowgen, it takes
>> >> the lock which guarantees everything is stable.
>> > If thread missed generation update, it is it.
>> >
>> > Fence would handle the other case, when the thread observed cowgen
>> > udate,
>> > but continue to use old cow values.
>> >
>> > The process lock does not help there at all.
>> >
>>
>> What do you mean by 'cow values'? You mean the pointers like p_ucred?
>>
>> td_ucred and td_limit are only modified by curthread, thus they don't
>> require synchronization. p_ucred and p_limit are only accessed with
>> the lock held, which provides the necessary synchronization. The only
>> thing inspected locklessly is the cowgen counter, which is safe to
>> race against.
> You are talking about removing the process lock, right in the commit
> message.  The only lock case related to cow values is around reading of
> the p_ pointers, to stash them into td_.

No, see 93288e2445fea95cb15b266759c2d9e382ea4e47 as an example
consumer. All work is still done with the process lock held. The
difference is that if the change at hand is the only one which needs
syncing for curthread, curthread can sync it immediately and end up
with up-to-date cowgen, consequently if no other changes pop up next
time it checks cowgen, it will avoid calling thread_cow_update and the
proc lock/unlock trip.

>>
>> >>
>> >> The code definitely should use atomic_store/load_int though, but there
>> >> are numerous bugs of this sort all over, so I don't think this is
>> >> pressing.
>> >>
>> >> >
>> >> >> +}
>> >> >> +
>> >> >>  /*
>> >> >>   * Discard the current thread and exit from its context.
>> >> >>   * Always called with scheduler locked.
>> >> >> diff --git a/sys/sys/proc.h b/sys/sys/proc.h
>> >> >> index ff97bfbd54a9..0e33192303f4 100644
>> >> >> --- a/sys/sys/proc.h
>> >> >> +++ b/sys/sys/proc.h
>> >> >> @@ -1009,6 +1009,14 @@ extern pid_t pid_max;
>> >> >>  	(p)->p_cowgen++;						\
>> >> >>  } while (0)
>> >> >>
>> >> >> +#define	PROC_COW_CHANGECOUNT(td, p) ({					\
>> >> >> +	struct thread *_td = (td);					\
>> >> >> +	struct proc *_p = (p);						\
>> >> >> +	MPASS(_td == curthread);					\
>> >> >> +	PROC_LOCK_ASSERT(_p, MA_OWNED);					\
>> >> >> +	_p->p_cowgen - _td->td_cowgen;					\
>> >> >> +})
>> >> >> +
>> >> >>  /* Check whether a thread is safe to be swapped out. */
>> >> >>  #define	thread_safetoswapout(td)	((td)->td_flags & TDF_CANSWAP)
>> >> >>
>> >> >> @@ -1200,6 +1208,7 @@ void	thread_cow_get_proc(struct thread
>> >> >> *newtd,
>> >> >> struct proc *p);
>> >> >>  void	thread_cow_get(struct thread *newtd, struct thread *td);
>> >> >>  void	thread_cow_free(struct thread *td);
>> >> >>  void	thread_cow_update(struct thread *td);
>> >> >> +void	thread_cow_synced(struct thread *td);
>> >> >>  int	thread_create(struct thread *td, struct rtprio *rtp,
>> >> >>  	    int (*initialize_thread)(struct thread *, void *), void
>> >> >> *thunk);
>> >> >>  void	thread_exit(void) __dead2;
>> >> >
>> >>
>> >>
>> >> --
>> >> Mateusz Guzik <mjguzik gmail.com>
>> >
>>
>>
>> --
>> Mateusz Guzik <mjguzik gmail.com>
>


-- 
Mateusz Guzik <mjguzik gmail.com>