Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: staging: ion: ION allocation fall back order depends on heap
 linkage order
To:     Alexey Skidanov <alexey.skidanov@gmail.com>,
        devel@driverdev.osuosl.org
Cc:     linux-kernel@vger.kernel.org
References: <CAM7Y4U8FmTNrn=BYN-J+_hVz7JTuaiV+y5Bao78wj3vYCPrz+w@mail.gmail.com>
From:   Laura Abbott <labbott@redhat.com>
Message-ID: <58951af2-a84e-7d7c-e956-ece3190aa8c2@redhat.com>
Date:   Tue, 6 Feb 2018 15:48:01 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <CAM7Y4U8FmTNrn=BYN-J+_hVz7JTuaiV+y5Bao78wj3vYCPrz+w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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