On Sun, May 05, 2019 at 11:13:01AM +0300, Gal Pressman wrote:
> 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..
It is, vzalloc isn't just kzalloc followed by vzalloc and you
shouldn't expec the two to be the same. Most likely the above has bad
behavior if it triggers reclaim.
Jason
