On 27-Mar-19 13:47, Leon Romanovsky wrote:
> On Wed, Mar 27, 2019 at 11:44:52AM +0200, Gal Pressman wrote:
>> On 27-Mar-19 11:33, Leon Romanovsky wrote:
>>> On Wed, Mar 27, 2019 at 11:01:21AM +0200, Gal Pressman wrote:
>>>> On 26-Mar-19 16:56, Leon Romanovsky wrote:
>>>>> From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>>>>>
>>>>> Hi,
>>>>>
>>>>> This is continuation of our conversion from driver allocations to core
>>>>> responsibility. Current series converts SRQ and AH, while latter
>>>>> required some code preparation.
>>>>>
>>>>> The most challenge was in supporting AH to be allocated with GFP_ATOMIC
>>>>> or GFP_KERNEL according to context. The common solution is to use in_atomic()
>>>>> and in_interrupt() already used in many drivers, but unfortunately
>>>>> checkpatch produces warning.
>>>>>
>>>>> WARNING: use of in_atomic() is incorrect outside core kernel code
>>>>> #37: FILE: include/rdma/ib_verbs.h:2288:
>>>>> + (in_atomic() || in_interrupt()) ? GFP_ATOMIC : GFP_KERNEL))
>>>>
>>>> Hi Leon,
>>>>
>>>> The existing method to test for create AH atomic flow is using the flags
>>>> parameters (RDMA_CREATE_AH_SLEEPABLE).
>>>> I suggest you add a gfp_flags parameter to rdma_zalloc_drv_obj() and pass
>>>> GFP_ATOMIC in case RDMA_CREATE_AH_SLEEPABLE flag is set.
>>>
>>> Hi Gal,
>>>
>>> It can be a possible solution too.
>>>
>>> Before I'm changing all rdma_zalloc_drv_obj() or introduce new similar helper,
>>> can you give me the rationale behind such change?
>>>
>>> Most of the objects need GFP_KERNEL allocations and I'm having a hard time
>>> to justify for myself the need "to punish" API just because of checkpatch
>>> warning, while this "automatic" mode does its job to determine context.
>>
>> I don't know if the checkpatch warning is justified or not but if there's an
>> existing way to identify the atomic flow then it should be used. Using
>> in_atomic() for some cases and the flags for other cases makes for inconsistent
>> code.
>>
>> Perhaps the "automatic" mode should also replace the 'flags' parameter, but IMO
>> as long as the flags exist they should be used.
>
> We can revert b090c4e3a07c ("RDMA: Mark if create address handle is in
> a sleepable context") if it is needed.
I disagree that removing existing functionality in order to add new
functionality is the solution. If you're going to provide an alternative with
your patch that sounds reasonable.
