On 03-May-19 15:19, Jason Gunthorpe wrote:
> On Fri, May 03, 2019 at 12:53:55PM +0300, Gal Pressman wrote:
>> On 02-May-19 21:14, Jason Gunthorpe wrote:
>>> On Wed, May 01, 2019 at 01:48:22PM +0300, Gal Pressman wrote:
>>>
>>>> +/* create a page buffer list from a mapped user memory region */
>>>> +static int pbl_create(struct efa_dev *dev,
>>>> + struct pbl_context *pbl,
>>>> + struct ib_umem *umem,
>>>> + int hp_cnt,
>>>> + u8 hp_shift)
>>>> +{
>>>> + int err;
>>>> +
>>>> + pbl->pbl_buf_size_in_bytes = hp_cnt * EFA_CHUNK_PAYLOAD_PTR_SIZE;
>>>> + pbl->pbl_buf = kzalloc(pbl->pbl_buf_size_in_bytes,
>>>> + GFP_KERNEL | __GFP_NOWARN);
>>>> + if (pbl->pbl_buf) {
>>>> + pbl->physically_continuous = 1;
>>>> + err = umem_to_page_list(dev, umem, pbl->pbl_buf, hp_cnt,
>>>> + hp_shift);
>>>> + if (err)
>>>> + goto err_continuous;
>>>> + err = pbl_continuous_initialize(dev, pbl);
>>>> + if (err)
>>>> + goto err_continuous;
>>>> + } else {
>>>> + pbl->physically_continuous = 0;
>>>> + pbl->pbl_buf = vzalloc(pbl->pbl_buf_size_in_bytes);
>>>> + if (!pbl->pbl_buf)
>>>> + return -ENOMEM;
>>>
>>> This way to fallback seems ugly, I think you should just call kvzalloc
>>> and check for continuity during the umem_to_page_list
>>
>> I've considered using kvzalloc, but it doesn't really fit this use case.
>
> It does, you just check for continuity when building the pbl instead
> of assuming it based on how it was created. It isn't hard, and drivers
> shouldn't abuse APIs like this
This is by no means abusing the API..
I honestly can't see how calling kvzalloc and trying to figure out whether the
buffer is continuous or not is better than a clear flow that asks for a
continuous buffer and falls back when it's not possible.
It would work, but there's no need to over complicate simple things.
