Re: [PATCH 12/44 take 2] [UBI] allocation unit implementation

[Arnd Bergmann]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/