Re: RFC: IRQ affinity (aka interrupt routing)

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

Thank you for your comments.

On 2015/07/17 5:18, Mindaugas Rasiukevicius wrote:
> 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.

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-nakahara%iij.ad.jp@localhost>