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

From: Mateusz Guzik <mjguzik_at_gmail.com>
Date: Sat, 12 Feb 2022 18:50:21 UTC
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.

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>