Re: [PATCH v5 04/14] softmmu/memory: Pass ram_flags to qemu_ram_alloc_from_fd()

[David Hildenbrand <david@redhat.com>]

On 20.04.21 12:45, Philippe Mathieu-Daudé wrote:
> On 4/20/21 12:36 PM, David Hildenbrand wrote:
>> On 20.04.21 12:18, Philippe Mathieu-Daudé wrote:
>>> Hi David,
>>>
>>> On 4/20/21 11:52 AM, Philippe Mathieu-Daudé wrote:
>>>> On 4/13/21 11:14 AM, David Hildenbrand wrote:
>>>>> Let's pass in ram flags just like we do with
>>>>> qemu_ram_alloc_from_file(),
>>>>> to clean up and prepare for more flags.
>>>>>
>>>>> Simplify the documentation of passed ram flags: Looking at our
>>>>> documentation of RAM_SHARED and RAM_PMEM is sufficient, no need to be
>>>>> repetitive.
>>>>>
>>>>> Reviewed-by: Peter Xu <peterx@redhat.com>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>    backends/hostmem-memfd.c | 7 ++++---
>>>>>    hw/misc/ivshmem.c        | 5 ++---
>>>>>    include/exec/memory.h    | 9 +++------
>>>>>    include/exec/ram_addr.h  | 6 +-----
>>>>>    softmmu/memory.c         | 7 +++----
>>>>>    5 files changed, 13 insertions(+), 21 deletions(-)
>>>>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>
>>>
>>> Actually it would be clearer to define the 0 value, maybe:
>>>
>>> #define RAM_NOFLAG     (0 << 0)
>>>
>>
>> This will also turn some code into
>>
>> ram_flags = backend->share ? RAM_SHARED : RAM_NOFLAG;
>> ram_flags |= backend->reserve ? RAM_NOFLAG : RAM_NORESERVE;
> 
> This is the callee view, withing the API, where you have all
> the context.
> 
>> Looking at other flag users, I barely see any such usage.
>> XKB_CONTEXT_NO_FLAGS, ALLOC_NO_FLAGS, and MEM_AFFINITY_NOFLAGS seem to
>> be the only ones. That's why I tend to not do it unless there are strong
>> opinions.
> 
> I'm more concerned about the caller perspective. What means this
> magic '0' in the arguments? Then I have to check the prototype.
> If the caller uses RAM_NO_FLAGS, I directly understand what is passed.
> 

Yeah, that makes sense. Even cleaner would be using the type system as 
we do in the kernel, e.g., for GFP flags

typedef unsigned int __bitwise gfp_t;
#define ___GFP_DMA	0x01u
#define __GFP_DMA	((__force gfp_t)___GFP_DMA)

And passing around gfp_t. Then even using "0" will bail out.

> Anyway my comment fits the usual "can be cleaned later" case.

Make sense, thanks.

-- 
Thanks,

David / dhildenb