Re: RFC: IRQ affinity (aka interrupt routing)

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

Hi,

On 2015/07/23 19:44, Kengo NAKAHARA wrote:
> 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?

There have been no objection for about 3 weeks.
I rebase and modify a little. Here is the updated patch.
    http://netbsd.org/~knakahara/intrctl/intrctl3.patch

Could you comment this patch?
If there is no objection, I will commit this patch after a few days or
weeks.


Thanks,

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

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

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