lkml.org 
[lkml]   [2012]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] edac: rewrite edac_align_ptr()
    On Wed, Apr 18, 2012 at 03:19:34PM -0300, Mauro Carvalho Chehab wrote:
    > The edac_align_ptr() function is used to prepare data for a single
    > memory allocation kzalloc() call. It counts how many bytes are needed
    > by some data structure.
    >
    > Using it as-is is not that trivial, as the quantity of memory elements
    > reserved is not there, but, instead, it is on a next call.
    >
    > In order to avoid mistakes when using it, move the number of allocated
    > elements into it, making easier to use it.
    >
    > Cc: Aristeu Rozanski <arozansk@redhat.com>
    > Cc: Doug Thompson <norsk5@yahoo.com>
    > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
    > ---
    >
    > v14: fixes a badly-solved rebase conflict, uses NULL instead of 0, adds more comments
    > and renames the counter for the number of structures to "count"
    >
    > drivers/edac/edac_device.c | 27 +++++++++++----------------
    > drivers/edac/edac_mc.c | 31 +++++++++++++++++++++++--------
    > drivers/edac/edac_module.h | 2 +-
    > drivers/edac/edac_pci.c | 6 +++---
    > 4 files changed, 38 insertions(+), 28 deletions(-)
    >
    > diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c
    > index 4b15459..cb397d9 100644
    > --- a/drivers/edac/edac_device.c
    > +++ b/drivers/edac/edac_device.c
    > @@ -79,7 +79,7 @@ struct edac_device_ctl_info *edac_device_alloc_ctl_info(
    > unsigned total_size;
    > unsigned count;
    > unsigned instance, block, attr;
    > - void *pvt;
    > + void *pvt, *p;
    > int err;
    >
    > debugf4("%s() instances=%d blocks=%d\n",
    > @@ -92,35 +92,30 @@ struct edac_device_ctl_info *edac_device_alloc_ctl_info(
    > * to be at least as stringent as what the compiler would
    > * provide if we could simply hardcode everything into a single struct.
    > */
    > - dev_ctl = (struct edac_device_ctl_info *)NULL;
    > + p = NULL;
    > + dev_ctl = edac_align_ptr(&p, sizeof(*dev_ctl), 1);
    >
    > /* Calc the 'end' offset past end of ONE ctl_info structure
    > * which will become the start of the 'instance' array
    > */
    > - dev_inst = edac_align_ptr(&dev_ctl[1], sizeof(*dev_inst));
    > + dev_inst = edac_align_ptr(&p, sizeof(*dev_inst), nr_instances);
    >
    > /* Calc the 'end' offset past the instance array within the ctl_info
    > * which will become the start of the block array
    > */
    > - dev_blk = edac_align_ptr(&dev_inst[nr_instances], sizeof(*dev_blk));
    > + count = nr_instances * nr_blocks;
    > + dev_blk = edac_align_ptr(&p, sizeof(*dev_blk), count);
    >
    > /* Calc the 'end' offset past the dev_blk array
    > * which will become the start of the attrib array, if any.
    > */
    > - count = nr_instances * nr_blocks;
    > - dev_attrib = edac_align_ptr(&dev_blk[count], sizeof(*dev_attrib));
    > -
    > - /* Check for case of when an attribute array is specified */
    > - if (nr_attrib > 0) {
    > - /* calc how many nr_attrib we need */
    > + /* calc how many nr_attrib we need */
    > + if (nr_attrib > 0)
    > count *= nr_attrib;
    > + dev_attrib = edac_align_ptr(&p, sizeof(*dev_attrib), count);
    >
    > - /* Calc the 'end' offset past the attributes array */
    > - pvt = edac_align_ptr(&dev_attrib[count], sz_private);
    > - } else {
    > - /* no attribute array specificed */
    > - pvt = edac_align_ptr(dev_attrib, sz_private);
    > - }
    > + /* Calc the 'end' offset past the attributes array */
    > + pvt = edac_align_ptr(&p, sz_private, 1);
    >
    > /* 'pvt' now points to where the private data area is.
    > * At this point 'pvt' (like dev_inst,dev_blk and dev_attrib)
    > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
    > index ffedae9..775a3ff 100644
    > --- a/drivers/edac/edac_mc.c
    > +++ b/drivers/edac/edac_mc.c
    > @@ -101,16 +101,28 @@ const char *edac_mem_types[] = {
    > };
    > EXPORT_SYMBOL_GPL(edac_mem_types);
    >
    > -/* 'ptr' points to a possibly unaligned item X such that sizeof(X) is 'size'.
    > +/**
    > + * edac_align_ptr - Prepares the pointer offsets for a single-shot allocation
    > + * @p: pointer to a pointer with the memory offset to be used. At
    > + * return, this will be incremented to point to the next offset
    > + * @size: Size of the data structure to be reserved
    > + * @count: Number of elements that should be reserved
    > + *
    > + * 'ptr' points to a possibly unaligned item X such that sizeof(X) is 'size'.

    There's no 'ptr' argument anymore. Also, the text doesn't apply anymore
    since the ptr is not possibly unaligned but the returned pointer *p is
    properly aligned to size * count.

    Also, this pointer is absolutely needed to keep the proper advancing
    further in memory to the proper offsets when allocating the struct along
    with its embedded structs, as edac_device_alloc_ctl_info() does it
    above, for example.

    > * Adjust 'ptr' so that its alignment is at least as stringent as what the
    > * compiler would provide for X and return the aligned result.
    > *
    > * If 'size' is a constant, the compiler will optimize this whole function
    > - * down to either a no-op or the addition of a constant to the value of 'ptr'.
    > + * down to either a no-op or the addition of a constant to the value of '*p'.
    > + *
    > + * At return, the pointer 'p' will be incremented.
    > */
    > -void *edac_align_ptr(void *ptr, unsigned size)
    > +void *edac_align_ptr(void **p, unsigned size, int count)

    'count' is non-descriptive and at least ambiguous as to what it relates
    to - call it 'n_elems' instead.

    > {
    > unsigned align, r;
    > + void *ptr = *p;
    > +
    > + *p += size * count;
    >
    > /* Here we assume that the alignment of a "long long" is the most
    > * stringent alignment that the compiler will ever provide by default.
    > @@ -132,6 +144,8 @@ void *edac_align_ptr(void *ptr, unsigned size)
    > if (r == 0)
    > return (char *)ptr;
    >
    > + *p += align - r;
    > +
    > return (void *)(((unsigned long)ptr) + align - r);
    > }

    In general, this edac_align_ptr is not really helpful because it requres
    the caller to know the exact layout of the struct it allocates memory
    for and what structs it has embedded. And frankly, I don't know how much
    it would help but I hear unaligned pointers are something bad on some
    !x86 architectures.

    Oh well...

    --
    Regards/Gruss,
    Boris.

    Advanced Micro Devices GmbH
    Einsteinring 24, 85609 Dornach
    GM: Alberto Bozzo
    Reg: Dornach, Landkreis Muenchen
    HRB Nr. 43632 WEEE Registernr: 129 19551


    \
     
     \ /
      Last update: 2012-04-23 16:09    [W:0.045 / U:0.132 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site