Re: RFC: IRQ affinity (aka interrupt routing)

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Hi,

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost> wrote:
> There has been no objection for about 10 months...
> So, I am going to commit this implementation. Here is the rebased
> patch,
>     http://netbsd.org/~knakahara/intrctl/intrctl.patch
> 
> If there is no objection, I commit this patch after a few days.

I have not really had time to do a proper review, but from a quick skim:

> +
> +	mutex_enter(&cpu_lock);
> +	ih = intr_get_handler(intrid);
> +	mutex_exit(&cpu_lock);
> +

How is cpu_lock supposed to help here?  You protect the iteration, but
not the actual handler structure once it is acquired.  It does not seem
to be MP-safe.

>
> +int
> +intr_construct_intrids(const kcpuset_t *cpuset, char ***intrids, int *count)
> +{
>

Might be worth to put the IDs and their count in a structure rather than
passing as parameters, but that is up to you.

> +intr_list_sysctl(SYSCTLFN_ARGS)
> ...
> +	buf = kmem_zalloc(*oldlenp, KM_SLEEP);
> +	if (buf == NULL)
> +		return ENOMEM;
> +
> +	ret = intr_list(buf, *oldlenp);
> +	if (ret < 0)
> +		return -ret;
> +
> +	return copyout(buf, oldp, *oldlenp);
> +}

This seems like a memory leak (buf is never freed).  Also, why negative
error numbers in intr_list() and elsewhere?

Note: kcpuset_copyin() returns an error code; although you zero the set
on creation, I think it is better to check for kcpuset_copyin() errors.

-- 
Mindaugas