On 05-Mar-19 03:39, Jason Gunthorpe wrote:
> On Wed, Feb 27, 2019 at 11:06:14AM +0200, Gal Pressman wrote:
>
>>>>>> +static struct scatterlist *efa_vmalloc_buf_to_sg(u64 *buf, int page_cnt)
>>>>>> +{
>>>>>> + struct scatterlist *sglist;
>>>>>> + struct page *pg;
>>>>>> + int i;
>>>>>> +
>>>>>> + sglist = kcalloc(page_cnt, sizeof(*sglist), GFP_KERNEL);
>>>>>> + if (!sglist)
>>>>>> + return NULL;
>>>>>> + sg_init_table(sglist, page_cnt);
>>>>>> + for (i = 0; i < page_cnt; i++) {
>>>>>> + pg = vmalloc_to_page(buf);
>>>>>> + if (!pg)
>>>>>> + goto err;
>>>>>> + WARN_ON_ONCE(PageHighMem(pg));
>>>>>
>>>>> Is this WARN_ON_ONCE() really an error that needs to be handled?
>>>>
>>>> AFAIK, there is no way we can actually get a higemem page here.
>>>> The WARN is here from early dev days, it should probably be removed.
>>>>
>>>>>
>>>>>
>>>>>> + sg_set_page(&sglist[i], pg, EFA_PAGE_SIZE, 0);
>>>>>> + buf = (u64 *)((u8 *)buf + EFA_PAGE_SIZE);
>>>
>>> Why do you need special EFA_PAGE_SIZE? Isn't PAGE_SIZE enough for you?
>>
>> EFA_PAGE_SIZE represents the device page size.
>
> Something is wrong here then, as vmalloc_to_page returns something
> that is *at most* PAGE_SIZE long. You can't go around and pass that
> into sg_set_page with EFA_PAGE_SIZE > PAGE_SIZE.
>
> Maybe this code is wrongly assuming PAGE_SIZE > EFA_PAGE_SIZE
> ?
We do assume PAGE_SIZE >= EFA_PAGE_SIZE.
For instance, on systems where PAGE_SIZE is 64k, we still use chunks of 4k.
>
> What you need here is to make the scatter list out of PAGE_SIZE blocks
> and then use Shiraz's work to fragment it into EFA_PAGE_SIZE blocks
> for building your pbl. dma_map_sg should use the largest sgls possible
> for efficiency.
>
> Also, I think there are now several places in the kernel converting
> vmalloc/kmalloc regions into scatterlist tables, it probably warrents
> adding a shared helper in lib/scatterlist.c that does a good and
> proper job of this.
My search yielded six occurrences, all under drivers/media.
Does this justify moving this function to scatterlist.c?
>
>>>>>> + buf = (u64 *)((u8 *)buf + EFA_PAGE_SIZE);
>
> Yuk
>
> buf += EFA_PAGE_SIZE/sizeof(*buf);
Done
