linux-kernel - Re: [PATCH] lib/ida: Document locking requirements a bit better

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: <20161026192525.qctwje64kyq7p3f3@phenom.ffwll.local>
Date:   Wed, 26 Oct 2016 21:25:25 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Tejun Heo <tj@...nel.org>
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>,
        LKML <linux-kernel@...r.kernel.org>,
        Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Michal Hocko <mhocko@...e.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Daniel Vetter <daniel.vetter@...el.com>
Subject: Re: [PATCH] lib/ida: Document locking requirements a bit better

On Wed, Oct 26, 2016 at 10:39:29AM -0400, Tejun Heo wrote:
> Hello, Daniel.
> 
> On Wed, Oct 26, 2016 at 04:27:39PM +0200, Daniel Vetter wrote:
> > I wanted to wrap a bunch of ida_simple_get calls into their own
> > locking, until I dug around and read the original commit message.
> > Stuff like this should imo be added to the kernel doc, let's do that.
> 
> Generally agreed but some nits below.

I value good docs but I suck at typing them ;-)

> > @@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get);
> >   * and go back to the ida_pre_get() call.  If the ida is full, it will
> >   * return %-ENOSPC.
> >   *
> > + * Note that callers must ensure that concurrent access to @ida is not possible.
> > + * When simplicity trumps concurrency needs look at ida_simple_get() instead.
> 
> Maybe we can make it a bit less dramatic?

What about?

"Note that callers must ensure that concurrent access to @ida is not possible.
See ida_simple_get() for a varaint which takes care of locking.
> 
> 
> > @@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy);
> >   * Allocates an id in the range start <= id < end, or returns -ENOSPC.
> >   * On memory allocation failure, returns -ENOMEM.
> >   *
> > + * Compared to ida_get_new_above() this function does its own locking and hence
> > + * is recommended everywhere where simplicity is preferred over the last bit of
> > + * speed.
> 
> Hmm... so, this isn't necessarily about speed.  For example, id
> allocation might have to happen inside a spinlock which protects a
> larger scope.  To guarantee GFP_KERNEL allocation behavior in such
> cases, the caller would have to call ida_pre_get() outside the said
> spinlock and then call ida_get_new_above() inside the lock.

Hm, ida_simple_get does that for you already ...

> I think it'd be better to explain what the simple version does and
> expects and then say that unless there are specific requirements using
> the simple version is recommended.

What about:

"Compared to ida_get_new_above() this function does its own locking, and
should be used unless there are special requirements."

-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch