From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46360) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCnN4-0005jI-U2 for qemu-devel@nongnu.org; Thu, 31 Jul 2014 06:14:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XCnMy-0000Rc-OT for qemu-devel@nongnu.org; Thu, 31 Jul 2014 06:14:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47312) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCnMy-0000RV-Db for qemu-devel@nongnu.org; Thu, 31 Jul 2014 06:13:56 -0400 Date: Thu, 31 Jul 2014 11:13:51 +0100 From: Stefan Hajnoczi Message-ID: <20140731101351.GD25929@stefanha-thinkpad.redhat.com> References: <1406759309-13742-1-git-send-email-jsnow@redhat.com> <1406759309-13742-4-git-send-email-jsnow@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="n2Pv11Ogg/Ox8ay5" Content-Disposition: inline In-Reply-To: <1406759309-13742-4-git-send-email-jsnow@redhat.com> Subject: Re: [Qemu-devel] [PATCH 3/4] libqos: add a simple first-fit memory allocator List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: marc.mari.barcelo@gmail.com, pbonzini@redhat.com, qemu-devel@nongnu.org --n2Pv11Ogg/Ox8ay5 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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)) =3D=3D (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 =3D PC_ALLOC_PARANOID | PC_ALLOC_LEAK_ASSERT; if (s->opts & mask =3D=3D mask) { (1) if ((node->addr !=3D s->start) || (node->size !=3D 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 se= t. Then in (2) we check whether PC_ALLOC_LEAK_ASSERT is set. But we already k= new that PC_ALLOC_LEAK_ASSERT must be set in (1), so I guess the logic should h= ave really been: if (s->opts & (PC_ALLOC_PARANOID | PC_ALLOC_LEAK_ASSERT)) { if ((node->addr !=3D s->start) || (node->size !=3D 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_E= NTNAME) > +#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 straightforw= ard 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; > =20 > typedef struct PCAlloc > { > QGuestAllocator alloc; > - > + PCAllocOpts opts; > uint64_t start; > uint64_t end; > + > + MemList used; > + MemList free; > } PCAlloc; > =20 > -static uint64_t pc_alloc(QGuestAllocator *allocator, size_t size) > +static inline void mlist_insert(MemList *head, MemBlock *insr) > { > - PCAlloc *s =3D 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 functio= ns are boilerplate. Why not use the QTAILQ macros directly? (It would be good to hide the list implementation if this was an external A= PI and you want to avoid exposing the implementation details, but within this source file there is no point in extra layers of indirection.) --n2Pv11Ogg/Ox8ay5 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJT2hbfAAoJEJykq7OBq3PIt8wH/AjoEftNqUI60933cd5q1OxI eAZXCqBkagg39i7cZVojNfNcVAxOREa1EEqp7e9k+WMhB5Zj4C/QgXC8cKgPj8KQ EkAi8Jx7omJXKUR4vCI61RnmYz4PjH68toPzUi62Rp7cdgme5QcCnpD5dh/OVjJL RAgCLIbEG3wdJK/UE2R5dZFpY66MZyseX2xpkHE71O4GszKJwa2mjH+jZlrYkaEs 2f1+rehB53+7SZDacuWziX1+Edh0fyZpQQEsmeRAGUqL9Q42iTNmvtp4zJN0+zg4 0x5+W+CrPXpkJemE1/b3EKh6XMgQe2dSquolRB6d7pjU9aAhx3OJyHYeidW54eg= =8pEF -----END PGP SIGNATURE----- --n2Pv11Ogg/Ox8ay5--