Re: git: eff9ee7c0c8e - main - hwpmc: Increase thread priority while iterating CPUs.

On 6/7/22, Alexander Motin <mav@freebsd.org> wrote:
> The branch main has been updated by mav:
>
> URL:
> https://cgit.FreeBSD.org/src/commit/?id=eff9ee7c0c8e1fe782d6c01a29bb23224b02a847
>
> commit eff9ee7c0c8e1fe782d6c01a29bb23224b02a847
> Author:     Alexander Motin <mav@FreeBSD.org>
> AuthorDate: 2022-06-07 02:36:16 +0000
> Commit:     Alexander Motin <mav@FreeBSD.org>
> CommitDate: 2022-06-07 02:51:01 +0000
>
>     hwpmc: Increase thread priority while iterating CPUs.
>
>     This allows to profile already running high-priority threads, that
>     otherwise by blocking thread migration to respective CPUs blocked PMC
>     management, i.e. profiling could start only when workload completed.
>
>     While there, return the thread to its original CPU after iterating
>     the list.  Otherwise all threads using PMC end up on the last CPU.
>

the iteration is a bogus scheme and needs to be replaced with
something similar to what rms locks are doing right now, see rms_rlock
and rms_wlock. Maybe I'll hack it up this month.

>     MFC after:      1 month
> ---
>  sys/dev/hwpmc/hwpmc_logging.c | 21 +++++++++------------
>  sys/dev/hwpmc/hwpmc_mod.c     | 17 ++++++++---------
>  sys/sys/pmc.h                 |  4 ++++
>  3 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/sys/dev/hwpmc/hwpmc_logging.c b/sys/dev/hwpmc/hwpmc_logging.c
> index c16adff8c842..8d9015af78e9 100644
> --- a/sys/dev/hwpmc/hwpmc_logging.c
> +++ b/sys/dev/hwpmc/hwpmc_logging.c
> @@ -764,6 +764,7 @@ pmclog_deconfigure_log(struct pmc_owner *po)
>  {
>  	int error;
>  	struct pmclog_buffer *lb;
> +	struct pmc_binding pb;
>
>  	PMCDBG1(LOG,CFG,1, "de-config po=%p", po);
>
> @@ -787,19 +788,16 @@ pmclog_deconfigure_log(struct pmc_owner *po)
>  		PMCLOG_RESET_BUFFER_DESCRIPTOR(lb);
>  		pmc_plb_rele(lb);
>  	}
> +	pmc_save_cpu_binding(&pb);
>  	for (int i = 0; i < mp_ncpus; i++) {
> -		thread_lock(curthread);
> -		sched_bind(curthread, i);
> -		thread_unlock(curthread);
> +		pmc_select_cpu(i);
>  		/* return the 'current' buffer to the global pool */
>  		if ((lb = po->po_curbuf[curcpu]) != NULL) {
>  			PMCLOG_RESET_BUFFER_DESCRIPTOR(lb);
>  			pmc_plb_rele(lb);
>  		}
>  	}
> -	thread_lock(curthread);
> -	sched_unbind(curthread);
> -	thread_unlock(curthread);
> +	pmc_restore_cpu_binding(&pb);
>
>  	/* drop a reference to the fd */
>  	if (po->po_file != NULL) {
> @@ -869,18 +867,17 @@ pmclog_schedule_one_cond(struct pmc_owner *po)
>  static void
>  pmclog_schedule_all(struct pmc_owner *po)
>  {
> +	struct pmc_binding pb;
> +
>  	/*
>  	 * Schedule the current buffer if any and not empty.
>  	 */
> +	pmc_save_cpu_binding(&pb);
>  	for (int i = 0; i < mp_ncpus; i++) {
> -		thread_lock(curthread);
> -		sched_bind(curthread, i);
> -		thread_unlock(curthread);
> +		pmc_select_cpu(i);
>  		pmclog_schedule_one_cond(po);
>  	}
> -	thread_lock(curthread);
> -	sched_unbind(curthread);
> -	thread_unlock(curthread);
> +	pmc_restore_cpu_binding(&pb);
>  }
>
>  int
> diff --git a/sys/dev/hwpmc/hwpmc_mod.c b/sys/dev/hwpmc/hwpmc_mod.c
> index e94527041af8..18b8bb1674a3 100644
> --- a/sys/dev/hwpmc/hwpmc_mod.c
> +++ b/sys/dev/hwpmc/hwpmc_mod.c
> @@ -249,9 +249,6 @@ static void	pmc_process_thread_delete(struct thread
> *td);
>  static void	pmc_process_thread_userret(struct thread *td);
>  static void	pmc_remove_owner(struct pmc_owner *po);
>  static void	pmc_remove_process_descriptor(struct pmc_process *pp);
> -static void	pmc_restore_cpu_binding(struct pmc_binding *pb);
> -static void	pmc_save_cpu_binding(struct pmc_binding *pb);
> -static void	pmc_select_cpu(int cpu);
>  static int	pmc_start(struct pmc *pm);
>  static int	pmc_stop(struct pmc *pm);
>  static int	pmc_syscall_handler(struct thread *td, void *syscall_args);
> @@ -739,13 +736,14 @@ pmc_ri_to_classdep(struct pmc_mdep *md, int ri, int
> *adjri)
>   * save the cpu binding of the current kthread
>   */
>
> -static void
> +void
>  pmc_save_cpu_binding(struct pmc_binding *pb)
>  {
>  	PMCDBG0(CPU,BND,2, "save-cpu");
>  	thread_lock(curthread);
>  	pb->pb_bound = sched_is_bound(curthread);
>  	pb->pb_cpu   = curthread->td_oncpu;
> +	pb->pb_priority = curthread->td_priority;
>  	thread_unlock(curthread);
>  	PMCDBG1(CPU,BND,2, "save-cpu cpu=%d", pb->pb_cpu);
>  }
> @@ -754,16 +752,16 @@ pmc_save_cpu_binding(struct pmc_binding *pb)
>   * restore the cpu binding of the current thread
>   */
>
> -static void
> +void
>  pmc_restore_cpu_binding(struct pmc_binding *pb)
>  {
>  	PMCDBG2(CPU,BND,2, "restore-cpu curcpu=%d restore=%d",
>  	    curthread->td_oncpu, pb->pb_cpu);
>  	thread_lock(curthread);
> -	if (pb->pb_bound)
> -		sched_bind(curthread, pb->pb_cpu);
> -	else
> +	sched_bind(curthread, pb->pb_cpu);
> +	if (!pb->pb_bound)
>  		sched_unbind(curthread);
> +	sched_prio(curthread, pb->pb_priority);
>  	thread_unlock(curthread);
>  	PMCDBG0(CPU,BND,2, "restore-cpu done");
>  }
> @@ -772,7 +770,7 @@ pmc_restore_cpu_binding(struct pmc_binding *pb)
>   * move execution over the specified cpu and bind it there.
>   */
>
> -static void
> +void
>  pmc_select_cpu(int cpu)
>  {
>  	KASSERT(cpu >= 0 && cpu < pmc_cpu_max(),
> @@ -784,6 +782,7 @@ pmc_select_cpu(int cpu)
>
>  	PMCDBG1(CPU,SEL,2, "select-cpu cpu=%d", cpu);
>  	thread_lock(curthread);
> +	sched_prio(curthread, PRI_MIN);
>  	sched_bind(curthread, cpu);
>  	thread_unlock(curthread);
>
> diff --git a/sys/sys/pmc.h b/sys/sys/pmc.h
> index 372e77ecdee7..18c38ca36659 100644
> --- a/sys/sys/pmc.h
> +++ b/sys/sys/pmc.h
> @@ -997,6 +997,7 @@ struct pmc_cpu {
>  struct pmc_binding {
>  	int	pb_bound;	/* is bound? */
>  	int	pb_cpu;		/* if so, to which CPU */
> +	u_char	pb_priority;	/* Thread active priority. */
>  };
>
>  struct pmc_mdep;
> @@ -1225,6 +1226,9 @@ int	pmc_save_kernel_callchain(uintptr_t *_cc, int
> _maxsamples,
>      struct trapframe *_tf);
>  int	pmc_save_user_callchain(uintptr_t *_cc, int _maxsamples,
>      struct trapframe *_tf);
> +void	pmc_restore_cpu_binding(struct pmc_binding *pb);
> +void	pmc_save_cpu_binding(struct pmc_binding *pb);
> +void	pmc_select_cpu(int cpu);
>  struct pmc_mdep *pmc_mdep_alloc(int nclasses);
>  void pmc_mdep_free(struct pmc_mdep *md);
>  uint64_t pmc_rdtsc(void);
>


-- 
Mateusz Guzik <mjguzik gmail.com>