Re: RFC: IRQ affinity (aka interrupt routing)

[Kengo NAKAHARA]

Hi,

On 2014/09/12 11:47, Kengo NAKAHARA wrote:
> (2014/08/20 18:06), Kengo NAKAHARA wrote:
>> I implement "intrctl" for amd64 and i386. Here is the implementation,
>> https://github.com/knakahara/netbsd-src/compare/rfc/irq-affinity2
>> and here is the patch
>> http://knakahara.github.io/patches/netbsd/irq-affinity-initctl.patch
>
> I implement MI version intrctl. Here is the implementation,
> https://github.com/knakahara/netbsd-src/compare/rfc/irq-affinity3
> and here is the patch
> http://knakahara.github.io/patches/netbsd/irq-affinity-mi.patch

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.


Thanks,

--
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA <k-nak...@iij.ad.jp>

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-...@muc.de

[Mindaugas Rasiukevicius]

Hi,

Kengo NAKAHARA <k-nak...@iij.ad.jp> 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

[Kengo NAKAHARA]

Hi,

Thank you for your comments.

On 2015/07/17 5:18, Mindaugas Rasiukevicius wrote:
> Kengo NAKAHARA <k-nak...@iij.ad.jp> 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.

I fix this reading side MP-unsafe issues.

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

I put IDs and number of IDs in a structure, and refactor whole function.

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

Because intr_list() returns the size of interrupts list data (used by
"intrctl(8) list") on success, and returns -1 * (error code) on fail.

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

I fix the this error check and above memory leak.

Furthermore, I review once more with my co-workers, and fix some issues.
Here is the updated patch
http://netbsd.org/~knakahara/intrctl/intrctl2.patch

Could you comments this patch?

Thanks,

--
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA <k-nak...@iij.ad.jp>