linux-kernel - Re: [PATCH 1/1] ipc/mqueue.c: Drag unneeded code out of locks

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]

Date:	Sat, 15 Nov 2014 13:22:19 -0800
From:	Davidlohr Bueso <dave@...olabs.net>
To:	Steven Stewart-Gallus <sstewartgallus00@...angara.bc.ca>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Manfred Spraul <manfred@...orfullife.com>,
	"J. Bruce Fields" <bfields@...hat.com>,
	Doug Ledford <dledford@...hat.com>,
	linux-newbie@...r.kernel.org
Subject: Re: [PATCH 1/1] ipc/mqueue.c: Drag unneeded code out of locks

On Sat, 2014-11-15 at 04:44 +0000, Steven Stewart-Gallus wrote:
> > What's the benefit here? Seems very risky at very little gain.
> >
> > The juice ain't worth the squeeze. NAK
> 
> Hello,
> 
> It is fair to argue that these changes are too tiny to be very
> meaningful for performance but the other goal of this patch was also
> to make the code look cleaner and easier for me and other people to
> understand. I hope that is a reasonable desire.

I don't see how on earth you consider your patch makes things easier to
understand. For instance, adding local variables from structures passed
to a function does *not* make things more clearer:

+                       bool too_many_open_files;
+                       long msgqueue_lim;
+                       unsigned long u_bytes;
+
+                       msgqueue_lim = rlimit(RLIMIT_MSGQUEUE);
+
+                       spin_lock(&mq_lock);
+
+                       u_bytes = u->mq_bytes;
+                       too_many_open_files = u_bytes + mq_bytes < u_bytes ||
+                               u_bytes + mq_bytes > msgqueue_lim;
+                       if (!too_many_open_files)

Plus you add context specific regions within the function (code around
{ }), ugly and something we've been removing!

In fact it makes it *harder* to read: Now you have to keep in mind where
each variable came from, ie: u_bytes.

> It is not fair to argue that these changes are risky. 

Oh no? Andrew already found issues with the patch. But you claim there
is no risk. But hey, not getting the patch right the first time is fine,
except that the patch (i) has no tangible effects on performance, (ii)
as a cleanup, it does not make it any easier to read, (iii) can
potentially introduce bugs (yes, extra risk in subtleties when changing
critical regions and goto statements... we have had similar regressions
in ipc in the past).

Thanks,
Davidlohr

--
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/