[lkml]   [2009]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 1/6] new kfifo API v0.3 - Code reorganization and move out spinlock
    The patch contents look good, but you still need to work on the
    changelog, see Documentation/SubmittingPatches. Sorry for the
    nitpicking, but I think it's important that you understand the

    On Friday 14 August 2009, Stefani Seibold wrote:
    > This is patch 1/6 of the new kfifo API:

    This line is useless as a changeset comment, it should be implied
    by the one-line summary. Regarding your summary, it reads

    [PATCH 1/6] new kfifo API v0.3 - Code reorganization and move out spinlock

    where it should be

    [PATCH 1/6] kfifo: move struct kfifo in place, move out spinlock

    It does raise the question why the two things are in one patch,
    and I admit that I misread your patch the last time, or I would
    have recommended splitting it in two. I guess it won't hurt too
    much to have them together. At least it saves you from patches
    all those files an extra time.

    Regarding the submission, Amerigo correctly pointed out that
    the mails should be in a single email thread, normally by
    making all the patches a reply to the [PATCH 0/X] mail.
    git-format-patch, git-send-email and quilt mail have options
    to do that automatically.

    > Remove spinlock
    > Change kfifo_put() into kfifo_put_locked() which requires an additional spinlock argument
    > Change kfifo_get() into kfifo_get_locked() which requires an additional spinlock argument
    > Make kfifo itself a member of the using data structure
    > kfifo_init() and kfifo_alloc() requieres now a kfifo-pointer argument and does not
    > longer allocated a kfifo structure

    The changelog describes *what* you do, which can be seen from the
    patch. Instead, it should say *why* you do it, as I mentioned before.

    E.g. the first line 'Remove spinlock' will make every reader wonder
    if that's really safe, and why it was there in the first place.

    How about this changelog:

    [PATCH] kfifo: move struct kfifo in place, move out spinlock

    This patch improves three areas of the simple kernel FIFO

    1. Since most users want to have the kfifo as part of another
    object, reorganize the code to allow including struct kfifo
    in another data structure. This requires changing the kfifo_alloc
    and kfifo_init prototypes so that we pass an existing kfifo
    pointer into them. This patch changes the implemenation and
    all existing users.

    2. Move the pointer to the spinlock out of struct kfifo. Most
    users in tree do not actually use a spinlock, so the few
    exceptions now have to call kfifo_{get,put}_locked, which takes
    an extra argument to a spinlock.

    3. Improve the prototypes of kfifo_get and kfifo_put to make
    the kerneldoc annotations more readable.

    > Lockless kfifo_len()
    > Fix "char *buffer" in prototypes into "from" respectively "to" to express the direction
    > Fix current user of the old kfifo API
    > drivers/char/nozomi.c | 21 +++---
    > drivers/char/sonypi.c | 49 +++++++--------
    > drivers/infiniband/hw/cxgb3/cxio_hal.h | 9 +-

    There is a line with '---' missing between the changelog and the
    diffstat. It's required for git-am and other tools so that the
    diffstat does not get into the history.

    >+ * __kfifo_len - returns the number of bytes available in the FIFO
    >+ * @fifo: the fifo to be used.
    >+ */
    >+static inline unsigned int __kfifo_len(struct kfifo *fifo)
    >+ register unsigned int out;
    >+ out = fifo->out;
    >+ barrier();
    >+ return fifo->in - out;

    This use of 'barrier()' is bogus. The plain barrier() only
    prevents gcc from reordering the object code, but in order to
    protect you from concurrent accesses on SMP systems, you really
    need 'smp_rmb()'. OTOH, I don't think that any barrier actually
    helps here, and that the old code was fine as well. Your change
    doesn't hurt, but it looks confusing.

    Arnd <><

     \ /
      Last update: 2009-08-14 14:23    [W:0.024 / U:11.460 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site