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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200908141421.09157.arnd@arndb.de>
Date:	Fri, 14 Aug 2009 14:21:09 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Stefani Seibold <stefani@...bold.net>
Cc:	"linux-kernel" <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	Amerigo Wang <xiyou.wangcong@...il.com>
Subject: Re: [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
rules.

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
implementation:

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