lkml.org 
[lkml]   [2007]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 12/44 take 2] [UBI] allocation unit implementation
    From
    Date
    On Sat, 2007-02-17 at 21:55 +0100, Arnd Bergmann wrote:
    > On Saturday 17 February 2007 17:55, Artem Bityutskiy wrote:
    > > +#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.

    I'll put them all to ubi.h to start with. I am still not convinced
    having one big header is more readable then per-unit headers.

    > > +#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.

    Done.

    > > +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.

    Good point, done. Left only wl_entries_slab, wl_prot_entry_slab, and
    eba_ltree_entry_slab.


    > > +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.

    Done.

    > > +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.

    I need a wrapper for VID header allocation, so better to leave this for
    symmetry.

    Thanks, Artem.

    --
    Best regards,
    Artem Bityutskiy (Битюцкий Артём)

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-02-19 12:09    [W:2.974 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site