lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 17 Feb 2007 21:55:12 +0100 From: Arnd Bergmann <arnd@...db.de> To: Artem Bityutskiy <dedekind@...radead.org> Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Christoph Hellwig <hch@...radead.org>, Frank Haverkamp <haver@...t.ibm.com>, Thomas Gleixner <tglx@...utronix.de>, David Woodhouse <dwmw2@...radead.org>, Josh Boyer <jwboyer@...ux.vnet.ibm.com> Subject: Re: [PATCH 12/44 take 2] [UBI] allocation unit implementation On Saturday 17 February 2007 17:55, Artem Bityutskiy wrote: > diff -auNrp tmp-from/drivers/mtd/ubi/alloc.c tmp-to/drivers/mtd/ubi/alloc.c > +#include "ubi.h" > +#include "alloc.h" > +#include "io.h" > +#include "background.h" > +#include "wl.h" > +#include "debug.h" > +#include "eba.h" > +#include "scan.h" I don't see much point in having one local header for each of these, you could simply put all of the declarations into one header in the ubi directory. > + > +#define BGT_WORK_SLAB_NAME "ubi_bgt_work_slab" > +#define WL_ERASE_WORK_SLAB_NAME "ubi_wl_erase_work_slab" > +#define WL_ENTRY_SLAB_NAME "ubi_wl_entry_slab" > +#define WL_PROT_ENTRY_SLAB_NAME "ubi_wl_prow_entry_slab" > +#define EBA_LTREE_ENTRY_SLAB_NAME "ubi_eba_ltree_entry_slab" > +#define SCAN_EB_SLAB_NAME "ubi_scan_leb" > +#define SCAN_VOLUME_SLAB_NAME "ubi_scan_volume" These macros seem rather pointless, each of them is only used once, and the macro name directly corresponds to the contents. > +static struct kmem_cache *bgt_work_slab; > +static struct kmem_cache *wl_erase_work_slab; > +static struct kmem_cache *wl_entries_slab; > +static struct kmem_cache *wl_prot_entry_slab; > +static struct kmem_cache *eba_ltree_entry_slab; > +static struct kmem_cache *scan_eb_slab; > +static struct kmem_cache *scan_volume_slab; Do you really need all these slab caches? If a cache only contains a small number of objects, e.g. one per volume, then you're much better off using a regular kmalloc. > +void *ubi_kzalloc(size_t size) > +{ > + void *ret; > + > + ret = kzalloc(size, GFP_KERNEL); > + if (unlikely(!ret)) { > + ubi_err("cannot allocate %zd bytes", size); > + dump_stack(); > + return NULL; > + } > + > + return ret; > +} > + > +void *ubi_kmalloc(size_t size) > +{ > + void *ret; > + > + ret = kmalloc(size, GFP_KERNEL); > + if (unlikely(!ret)) { > + ubi_err("cannot allocate %zd bytes", size); > + dump_stack(); > + return NULL; > + } > + > + return ret; > +} > + > +void ubi_kfree(const void *obj) > +{ > + if (unlikely(!obj)) > + return; > + kfree(obj); > +} These look somewhat too complex. Don't introduce your own generic infrastructure if you can help it. IIRC, when kmalloc fails, you already get the full stack trace from the buddy allocator, so this is just duplication. Better use the regular kzalloc/kfree calls directly. > +struct ubi_ec_hdr *ubi_zalloc_ec_hdr(const struct ubi_info *ubi) > +{ > + struct ubi_ec_hdr *ec_hdr; > + const struct ubi_io_info *io = ubi->io; > + > + ec_hdr = kzalloc(io->ec_hdr_alsize, GFP_KERNEL); > + if (unlikely(!ec_hdr)) { > + ubi_err("cannot allocate %d bytes", io->ec_hdr_alsize); > + dump_stack(); > + return NULL; > + } > + > + return ec_hdr; > +} > + > +void ubi_free_ec_hdr(const struct ubi_info *ubi, struct ubi_ec_hdr *ec_hdr) > +{ > + if (unlikely(!ec_hdr)) > + return; > + kfree(ec_hdr); > +} same for this and the others. Unless the allocation is done in many places in the code from a single slab cache, just call kmem_cache_alloc or kmalloc directly. Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists