linux-kernel - Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

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: <20171211224301.GA3925@bombadil.infradead.org>
Date:   Mon, 11 Dec 2017 14:43:01 -0800
From:   Matthew Wilcox <willy@...radead.org>
To:     Joe Perches <joe@...ches.com>
Cc:     Dave Chinner <david@...morbit.com>,
        Alan Stern <stern@...land.harvard.edu>,
        Byungchul Park <byungchul.park@....com>,
        Theodore Ts'o <tytso@....edu>,
        Matthew Wilcox <mawilcox@...rosoft.com>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Jens Axboe <axboe@...nel.dk>,
        Rehas Sachdeva <aquannie@...il.com>, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-nilfs@...r.kernel.org, linux-btrfs@...r.kernel.org,
        linux-xfs@...r.kernel.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-team@....com
Subject: Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

On Mon, Dec 11, 2017 at 02:12:28PM -0800, Joe Perches wrote:
> Completely reasonable.  Thanks.

If we're doing "completely reasonable" complaints, then ...

 - I don't understand why plain 'unsigned' is deemed bad.

 - The rule about all function parameters in prototypes having a name
   doesn't make sense.  Example:

int ida_get_new_above(struct ida *ida, int starting_id, int *p_id);

   There is zero additional value in naming 'ida'.  I know it's an ida.
   The struct name tells me that.  If there're two struct ida pointers
   in the prototype, then sure, I want to name them so I know which is
   which (maybe 'src' and 'dst').  Having an unadorned 'int' parameter
   to a function should be a firable offence.  But there's no need to
   call 'gfp_t' anything.  We know it's a gfp_t.  Adding 'gfp_mask'
   after it doesn't tell us anything extra.

 - Forcing a blank line after variable declarations sometimes makes for
   some weird-looking code.  For example, there is no problem with this
   code (from a checkpatch PoV):

        if (xa_is_sibling(entry)) {
                offset = xa_to_sibling(entry);
                entry = xa_entry(xas->xa, node, offset);
                /* Move xa_index to the first index of this entry */
                xas->xa_index = (((xas->xa_index >> node->shift) &
                                  ~XA_CHUNK_MASK) | offset) << node->shift;
        }

   but if I decide I don't need 'offset' outside this block, and I want
   to move the declaration inside, it looks like this:

        if (xa_is_sibling(entry)) {
                unsigned int offset = xa_to_sibling(entry);

                entry = xa_entry(xas->xa, node, offset);
                /* Move xa_index to the first index of this entry */
                xas->xa_index = (((xas->xa_index >> node->shift) &
                                  ~XA_CHUNK_MASK) | offset) << node->shift;
        }

   Does that blank line really add anything to your comprehension of the
   block?  It upsets my train of thought.

   Constructively, I think this warning can be suppressed for blocks
   that are under, say, 8 lines.  Or maybe indented blocks is where I don't
   want this warning.  Not sure.

   Here's another example where I don't think the blank line adds anything:

static inline int xa_store_empty(struct xarray *xa, unsigned long index,
                void *entry, gfp_t gfp, int errno)
{
        void *curr = xa_cmpxchg(xa, index, NULL, entry, gfp);
        if (!curr)
                return 0;
        if (xa_is_err(curr))
                return xa_err(curr);
        return errno;
}

   So line count definitely has something to do with it.

 - There's no warning for the first paragraph of section 6:

6) Functions
------------

Functions should be short and sweet, and do just one thing.  They should
fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
as we all know), and do one thing and do that well.

   I'm not expecting you to be able to write a perl script that checks
   the first line, but we have way too many 200-plus line functions in
   the kernel.  I'd like a warning on anything over 200 lines (a factor
   of 4 over Linus's stated goal).

 - I don't understand the error for xa_head here:

struct xarray {
        spinlock_t      xa_lock;
        gfp_t           xa_flags;
        void __rcu *    xa_head;
};

   Do people really think that:

struct xarray {
        spinlock_t      xa_lock;
        gfp_t           xa_flags;
        void __rcu	*xa_head;
};

   is more aesthetically pleasing?  And not just that, but it's an *error*
   so the former is *RIGHT* and this is *WRONG*.  And not just a matter
   of taste?