svn commit: r317756 - in head/sys: kern sys

[John Baldwin]

On Wednesday, May 03, 2017 06:41:08 PM Conrad Meyer wrote:
> Author: cem
> Date: Wed May  3 18:41:08 2017
> New Revision: 317756
> URL: https://svnweb.freebsd.org/changeset/base/317756
> 
> Log:
>   Extend cpuset_get/setaffinity() APIs
>   
>   Add IRQ placement-only and ithread-only API variants. intr_event_bind
>   has been extended with sibling methods, as it has many more callsites in
>   existing code.
>
>   Reviewed by:	kib@, adrian@ (earlier version)
>   Sponsored by:	Dell EMC Isilon
>   Differential Revision:	https://reviews.freebsd.org/D10586
> 
> Modified: head/sys/kern/kern_intr.c
> ==============================================================================
> --- head/sys/kern/kern_intr.c	Wed May  3 17:21:01 2017	(r317755)
> +++ head/sys/kern/kern_intr.c	Wed May  3 18:41:08 2017	(r317756)
> @@ -287,13 +287,11 @@ intr_event_create(struct intr_event **ev
>  /*
>   * Bind an interrupt event to the specified CPU.  Note that not all
>   * platforms support binding an interrupt to a CPU.  For those
> - * platforms this request will fail.  For supported platforms, any
> - * associated ithreads as well as the primary interrupt context will
> - * be bound to the specificed CPU.  Using a cpu id of NOCPU unbinds
> + * platforms this request will fail.  Using a cpu id of NOCPU unbinds
>   * the interrupt event.
>   */
> -int
> -intr_event_bind(struct intr_event *ie, int cpu)
> +static int
> +_intr_event_bind(struct intr_event *ie, int cpu, bool bindirq, bool bindithread)
>  {
>  	lwpid_t id;
>  	int error;
> @@ -313,35 +311,75 @@ intr_event_bind(struct intr_event *ie, i
>  	 * If we have any ithreads try to set their mask first to verify
>  	 * permissions, etc.
>  	 */
> -	mtx_lock(&ie->ie_lock);
> -	if (ie->ie_thread != NULL) {
> -		id = ie->ie_thread->it_thread->td_tid;
> -		mtx_unlock(&ie->ie_lock);
> -		error = cpuset_setithread(id, cpu);
> -		if (error)
> -			return (error);
> -	} else
> -		mtx_unlock(&ie->ie_lock);
> -	error = ie->ie_assign_cpu(ie->ie_source, cpu);
> -	if (error) {
> +	if (bindithread) {
>  		mtx_lock(&ie->ie_lock);
>  		if (ie->ie_thread != NULL) {
> -			cpu = ie->ie_cpu;
>  			id = ie->ie_thread->it_thread->td_tid;
>  			mtx_unlock(&ie->ie_lock);
> -			(void)cpuset_setithread(id, cpu);
> +			error = cpuset_setithread(id, cpu);
> +			if (error)
> +				return (error);
>  		} else
>  			mtx_unlock(&ie->ie_lock);
> +	}
> +	if (bindirq)
> +		error = ie->ie_assign_cpu(ie->ie_source, cpu);
> +	if (error) {
> +		if (bindithread) {

This error block should probably be under 'if (bindirq)' as it only used
to "undo" an ithread binding if an irq binding failed.

> +			mtx_lock(&ie->ie_lock);
> +			if (ie->ie_thread != NULL) {
> +				cpu = ie->ie_cpu;
> +				id = ie->ie_thread->it_thread->td_tid;
> +				mtx_unlock(&ie->ie_lock);
> +				(void)cpuset_setithread(id, cpu);
> +			} else
> +				mtx_unlock(&ie->ie_lock);
> +		}
>  		return (error);
>  	}
>  
> -	mtx_lock(&ie->ie_lock);
> -	ie->ie_cpu = cpu;
> -	mtx_unlock(&ie->ie_lock);
> +	if (bindirq) {
> +		mtx_lock(&ie->ie_lock);
> +		ie->ie_cpu = cpu;
> +		mtx_unlock(&ie->ie_lock);
> +	}

This would then be in the else.  That is, the code structure would be
something like:

  if (bindithread) {
     /* new code to bind ithread */
  }
  if (bindirq) {
     error = ie_assign_cpu();
     if (error) {
        /* undo bind ithread */
     } else {
        /* set ie_cpu */
     }
  }

>  
>  	return (error);
>  }
>  
> +/*
> + * Bind an interrupt event to the specified CPU.  For supported platforms, any
> + * associated ithreads as well as the primary interrupt context will be bound
> + * to the specificed CPU.
> + */
> +int
> +intr_event_bind(struct intr_event *ie, int cpu)
> +{
> +
> +	return (_intr_event_bind(ie, cpu, true, true));
> +}

This is used by new-bus so the wrapper needs to stay.

> +
> +/*
> + * Bind an interrupt event to the specified CPU, but do not bind associated
> + * ithreads.
> + */
> +int
> +intr_event_bind_irqonly(struct intr_event *ie, int cpu)
> +{
> +
> +	return (_intr_event_bind(ie, cpu, true, false));
> +}
> +
> +/*
> + * Bind an interrupt event's ithread to the specified CPU.
> + */
> +int
> +intr_event_bind_ithread(struct intr_event *ie, int cpu)
> +{
> +
> +	return (_intr_event_bind(ie, cpu, false, true));
> +}

These aren't otherwise used?  Perhaps rather than having these wrappers, the
switch in intr_setaffinity() could just call _intr_event_bind() directly
with the right set of booleans?  The compiler will probably simplify
it to the equivalent of:

   _intr_event_bind(ie, cpu, which != CPU_WHICH_ITHREAD,
       which != CPU_WHICH_INTRHANDLER);

-- 
John Baldwin