Re: [Qemu-devel] [PATCH 3/4] libqos: add a simple first-fit memory allocator

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3803 bytes --]


On 07/31/2014 06:13 AM, Stefan Hajnoczi wrote:
> On Wed, Jul 30, 2014 at 06:28:28PM -0400, John Snow wrote:
>> +#define bitany(X, MASK) ((X) & (MASK))
>> +#define bitset(X, MASK) (bitany((X), (MASK)) == (MASK))
> This is subjective but macros like this should be avoided.  This macro does not
> encapsulate anything substantial.  It forces the reader to jump to the
> definition of this macro to understand the code, making code harder to read.
>
> IMO a cleaner solution is to drop the macros:
>
>    PCAllocOpts mask = PC_ALLOC_PARANOID | PC_ALLOC_LEAK_ASSERT;
>    if (s->opts & mask == mask) {                                 (1)
>        if ((node->addr != s->start) ||
>            (node->size != s->end - s->start)) {
>            fprintf(stderr, "Free list is corrupted.\n");
>            if (s->opts & PC_ALLOC_LEAK_ASSERT) {                 (2)
>                g_assert_not_reached();
>            }
>        }
>    }
>
> Now that I read the expanded code, a bug becomes exposed:
>
> In (1) we check that PC_ALLOC_PARANOID and PC_ALLOC_LEAK_ASSERT are both set.
> Then in (2) we check whether PC_ALLOC_LEAK_ASSERT is set.  But we already knew
> that PC_ALLOC_LEAK_ASSERT must be set in (1), so I guess the logic should have
> really been:
>
>    if (s->opts & (PC_ALLOC_PARANOID | PC_ALLOC_LEAK_ASSERT)) {
>        if ((node->addr != s->start) ||
>            (node->size != s->end - s->start)) {
>            fprintf(stderr, "Free list is corrupted.\n");
>            if (s->opts & PC_ALLOC_LEAK_ASSERT) {
>                g_assert_not_reached();
>            }
>        }
>    }
>
>> +#define MLIST_ENTNAME entries
>> +#define MLIST_FOREACH(node, head) QTAILQ_FOREACH((node), (head), MLIST_ENTNAME)
>> +#define MLIST_PREV(node) QTAILQ_PREV((node), MemList, MLIST_ENTNAME);
>> +#define MLIST_NEXT(node) QTAILQ_NEXT((node), MLIST_ENTNAME);
> For the same reasons as my previous comment, please don't hide straightforward
> expressions behind a macro.
>
>> +typedef QTAILQ_HEAD(MemList, MemBlock) MemList;
>> +typedef struct MemBlock {
>> +    QTAILQ_ENTRY(MemBlock) MLIST_ENTNAME;
>> +    uint64_t size;
>> +    uint64_t addr;
>> +} MemBlock;
>>   
>>   typedef struct PCAlloc
>>   {
>>       QGuestAllocator alloc;
>> -
>> +    PCAllocOpts opts;
>>       uint64_t start;
>>       uint64_t end;
>> +
>> +    MemList used;
>> +    MemList free;
>>   } PCAlloc;
>>   
>> -static uint64_t pc_alloc(QGuestAllocator *allocator, size_t size)
>> +static inline void mlist_insert(MemList *head, MemBlock *insr)
>>   {
>> -    PCAlloc *s = container_of(allocator, PCAlloc, alloc);
>> -    uint64_t addr;
>> +    QTAILQ_INSERT_HEAD(head, insr, MLIST_ENTNAME);
>> +}
>> +
>> +static inline void mlist_append(MemList *head, MemBlock *node)
>> +{
>> +    QTAILQ_INSERT_TAIL(head, node, MLIST_ENTNAME);
>> +}
>> +
>> +static inline void mlist_unlink(MemList *head, MemBlock *rem)
>> +{
>> +    QTAILQ_REMOVE(head, rem, MLIST_ENTNAME);
>> +}
> For the same reasons as my comments about the macros, these trivial functions
> are boilerplate.  Why not use the QTAILQ macros directly?
For /at least/ the append and insert cases, I desired call-by-value 
semantics.
As a matter of taste, I find macros annoying for the reason that you 
cannot inline things such as:
mlist_insert(list, mlist_new(...));

but unlink is certainly superfluous, and just something I did for some 
consistency.

If there is a matter of style where in-line function call is to be 
avoided entirely, I'll just nix all of these trivial inlines.
>
> (It would be good to hide the list implementation if this was an external API
> and you want to avoid exposing the implementation details, but within this
> source file there is no point in extra layers of indirection.)
Force of habit, but you're right. I'm not exporting the interface.

[-- Attachment #2: Type: text/html, Size: 4558 bytes --]