linux-kernel - Re: [PATCH] blk: clean up plug

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: <20150417210050.GA819331@devbig257.prn2.facebook.com>
Date:	Fri, 17 Apr 2015 14:00:51 -0700
From:	Shaohua Li <shli@...com>
To:	Jeff Moyer <jmoyer@...hat.com>
CC:	<linux-kernel@...r.kernel.org>, <axboe@...nel.dk>
Subject: Re: [PATCH] blk: clean up plug

On Fri, Apr 17, 2015 at 04:54:40PM -0400, Jeff Moyer wrote:
> Shaohua Li <shli@...com> writes:
> 
> > Current code looks like inner plug gets flushed with a
> > blk_finish_plug(). Actually it's a nop. All requests/callbacks are added
> > to current->plug, while only outmost plug is assigned to current->plug.
> > So inner plug always has empty request/callback list, which makes
> > blk_flush_plug_list() a nop. This tries to make the code more clear.
> >
> > Signed-off-by: Shaohua Li <shli@...com>
> 
> Hi, Shaohua,
> 
> I agree that this looks like a clean-up with no behavioral change, and
> it looks good to me.  However,it does make me scratch my head about the
> numbers I was seeing.  Here's the table from that other email thread[1]:
> 
> device|  vanilla   |    patch1      |   dio-noplug  |  noflush-nested
> ------+------------+----------------+---------------+-----------------
> rssda | 701,684    |    1,168,527   |   1,342,177   |   1,297,612
>       |     100%   |         +66%   |        +91%   |        +85%
> vdb0  | 358,264    |      902,913   |     906,850   |     922,327
>       |     100%   |        +152%   |       +153%   |       +157%
> 
> Patch1 refers to the first patch in this series, which fixes the merge
> logic for single-queue blk-mq devices.  Each column after that includes
> that first patch.  In dio-noplug, I removed the blk_plug from the
> direct-io code path (so there is no nesting at all).  This is a control,
> since it is what I expect the outcome of the noflush-nested column to
> actually be.  Then, the noflush-nested column leaves the blk_plug in
> place in the dio code, but includes the patch that prevents nested
> blk_plug's from being flushed.  All numbers are the average of 5 runs.
> With the exception of the vanilla run on rssda (the first run was
> faster, causing the average to go up), the standard deviation is very
> small.
> 
> For the dio-noplug column, if the inner plug really was a noop, then why
> would we see any change in performance?  Like I said, I agree with your
> reading of the code and the patch.  Color me confused.  I'll poke at it
> more next week.  For now, I think your patch is fine.
> 
> Reviewed-by: Jeff Moyer <jmoyer@...hat.com>

Thanks! I don't know why either the your second makes change.
 
> Also, Jens or Shaohua or anyone, please review my blk-mq plug fix (patch
> 1/2 of aforementioned thread).  ;)

You are not alone :), I posted 2 times too
http://marc.info/?l=linux-kernel&m=142627559617005&w=2


Thanks,
Shaohua
--
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/