Re: RFC: IRQ affinity (aka interrupt routing)

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

Hi,

On 2015/08/11 10:59, Kengo NAKAHARA wrote:
> 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.

I committed 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>