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

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.

> 
> 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>