lkml.org 
[lkml]   [2008]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch] block layer: kmemcheck fixes

    * Linus Torvalds <torvalds@linux-foundation.org> wrote:

    > On Thu, 7 Feb 2008, Ingo Molnar wrote:
    > > INIT_HLIST_NODE(&rq->hash);
    > > RB_CLEAR_NODE(&rq->rb_node);
    > > - rq->ioprio = 0;
    > > - rq->buffer = NULL;
    > > - rq->ref_count = 1;
    > > - rq->q = q;
    > > - rq->special = NULL;
    > > - rq->data_len = 0;
    > > - rq->data = NULL;
    > > - rq->nr_phys_segments = 0;
    > > - rq->sense = NULL;
    > > - rq->end_io = NULL;
    > > - rq->end_io_data = NULL;
    > > - rq->completion_data = NULL;
    > > - rq->next_rq = NULL;
    > > + rq->completion_data = NULL;
    > > + /* rq->elevator_private */
    > > + /* rq->elevator_private2 */
    > > + /* rq->rq_disk */
    > > + /* rq->start_time */
    > > + rq->nr_phys_segments = 0;
    > > + /* rq->nr_hw_segments */
    > > + rq->ioprio = 0;
    > > + rq->special = NULL;
    > > + rq->buffer = NULL;
    > ...
    >
    > Can we please just stop doing these one-by-one assignments, and just do
    > something like
    >
    > memset(rq, 0, sizeof(*rq));
    > rq->q = q;
    > rq->ref_count = 1;
    > INIT_HLIST_NODE(&rq->hash);
    > RB_CLEAR_NODE(&rq->rb_node);
    >
    > instead?
    >
    > The memset() is likely faster and smaller than one-by-one assignments
    > anyway, even if the one-by-ones can avoid initializing some field or
    > there ends up being a double initialization..

    i definitely agree and do that for all code i write.

    But if someone does item by item initialization for some crazy
    performance reason (networking folks tend to have such constructs), it
    should be done i think how i've done it in the patch: by systematically
    listing _every_ field in the structure, in the same order, and
    indicating it clearly when it is not initialized and why.

    and there it already shows that we do not initialize a few other members
    that could cause problems later on:

    + rq->data_len = 0;
    + /* rq->sense_len */
    + rq->data = NULL;
    + rq->sense = NULL;

    why is sense_len not initialized - while data_len is? In any case, these
    days the memclear instructions are dirt cheap and we should just always
    initialize everything to zero by default, especially if it's almost all
    zero-initialized anyway.

    Ingo


    \
     
     \ /
      Last update: 2008-02-07 20:35    [W:0.039 / U:0.024 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site