Re: staging: ion: ION allocation fall back order depends on heap linkage order

[Laura Abbott]

On 01/28/2018 08:24 AM, Alexey Skidanov wrote:
> Hi,
>
> According to my understanding, the allocation fall back order
> completely depends on heap->id that is assigned during the heap
> creation:
>     plist_for_each_entry(heap, &dev->heaps, node) {
>          /* if the caller didn't specify this heap id */
>          if (!((1 << heap->id) & heap_id_mask))
>              continue;
>          buffer = ion_buffer_create(heap, dev, len, flags);
>          if (!IS_ERR(buffer))
>              break;
>      }
>
> On creation, each heap is added to the priority list according to the
> priority assigned:
>
> ...
> static int heap_id;
> ...
> void ion_device_add_heap(struct ion_heap *heap)
> {
>          ...
>          heap->id = heap_id++;
>          ...
> }
>
>
> The order of creation is the order of linkage defined in the Makefile.
> Thus, by default, we have:
>
> heap id 2, type ION_HEAP_TYPE_DMA
> heap id 1, type ION_HEAP_TYPE_SYSTEM
> heap id 0, type ION_HEAP_TYPE_SYSTEM_CONTIG
>
> Changing the linkage order:
> diff --git a/drivers/staging/android/ion/Makefile
> b/drivers/staging/android/ion/Makefile
> index bb30bf8..e05052c 100644
> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile
> @@ -1,6 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0
>   obj-$(CONFIG_ION) +=   ion.o ion-ioctl.o ion_heap.o
> -obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o
>   obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
>   obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
>   obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
> +obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o
>
> I get the following order:
>
> heap id 2, type ION_HEAP_TYPE_SYSTEM
> heap id 1, type ION_HEAP_TYPE_SYSTEM_CONTIG
> heap id 0, type ION_HEAP_TYPE_DMA
>
> So, if the user specifies more than 1 heap in the heap_id_mask during
> allocation, the allocation fall back order completely depends on the
> order of linkage. Probably, it's better to let the user to define the
> fall back order (and NOT to be dependent on the linkage order at all)
> ?

Yup, you've hit upon a key problem. Having fallbacks be stable
was always a problem and the recommendation these days is to
not rely on them. You can specify a heap at a time and fallback
manually if you want that behavior.

If you have a proposal to make fallbacks work reliably without
overly complicating the ABI I'm happy to review it.

Thanks,
Laura