All of lore.kernel.org
 help / color / mirror / Atom feed
From: Julia Cartwright <julia@ni.com>
To: Alexander Stein <alexander.stein@systec-electronic.com>
Cc: linux-rt-users@vger.kernel.org, bigeasy@linutronix.de,
	tglx@linutronix.de
Subject: Re: [PATCH 1/1] squashfs: Disable "percpu multiple decompressor" on RT
Date: Thu, 3 May 2018 10:48:34 -0500	[thread overview]
Message-ID: <20180503154834.GF967@jcartwri.amer.corp.natinst.com> (raw)
In-Reply-To: <3619069.dCv7WPLmuO@ws-stein>

Hey Alexander-

On Thu, May 03, 2018 at 08:36:03AM +0200, Alexander Stein wrote:
> On Wednesday, May 2, 2018, 4:37:29 PM CEST Julia Cartwright wrote:
> > [..]
> > > +++ b/fs/squashfs/Kconfig
> > > @@ -86,6 +86,7 @@ config SQUASHFS_DECOMP_MULTI
> > >  
> > >  config SQUASHFS_DECOMP_MULTI_PERCPU
> > >  	bool "Use percpu multiple decompressors for parallel I/O"
> > > +	depends on !PREEMPT_RT_BASE
> > 
> > Hmm, I think we'd like to get out of the business of disabling Kconfig
> > options unless we are absolutely not given any other choice.
> > 
> > Looking at the codepaths involved in this squashfs decompressor, it
> > seems like this is a perfect candidate for the usage of local locks.
> > Can you give the following patch a try instead?
> 
> Sure. See below!

Thanks for giving it a test.

> > --- a/fs/squashfs/decompressor_multi_percpu.c
> > +++ b/fs/squashfs/decompressor_multi_percpu.c
> > @@ -6,6 +6,7 @@
> >   * the COPYING file in the top-level directory.
> >   */
> >  
> > +#include <linux/locallock.h>
> >  #include <linux/types.h>
> >  #include <linux/slab.h>
> >  #include <linux/percpu.h>
>
> <linux/locallock.h> needs to be added after <linux/slab.h>.

That seems broken.

> > [...]
> > @@ -79,10 +82,15 @@ int squashfs_decompress(struct squashfs_sb_info *msblk, struct buffer_head **bh,
> >  {
> >  	struct squashfs_stream __percpu *percpu =
> >  			(struct squashfs_stream __percpu *) msblk->stream;
> > -	struct squashfs_stream *stream = get_cpu_ptr(percpu);
> > -	int res = msblk->decompressor->decompress(msblk, stream->stream, bh, b,
> > -		offset, length, output);
> > -	put_cpu_ptr(stream);
> > +	struct squashfs_stream *stream;
> > +	int res;
> > +
> > +	stream = get_locked_var(stream_lock, percpu);
> > +
> > +	res = msblk->decompressor->decompress(msblk, stream->stream, bh, b,
> > +			offset, length, output);
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This doesn't work. I get a "Unable to handle kernel paging request at virtual address bcf67c1c". addr2line says its this line.

Hrmph.  I was lost in the percpu arithmetic.  Should have built with
sparse, it clearly tells me I screwed up.  Are you able to give this a
try?  It's sparse-clean now :)

   Julia

diff --git a/fs/squashfs/decompressor_multi_percpu.c b/fs/squashfs/decompressor_multi_percpu.c
index 23a9c28ad8ea..6a73c4fa88e7 100644
--- a/fs/squashfs/decompressor_multi_percpu.c
+++ b/fs/squashfs/decompressor_multi_percpu.c
@@ -10,6 +10,7 @@
 #include <linux/slab.h>
 #include <linux/percpu.h>
 #include <linux/buffer_head.h>
+#include <linux/locallock.h>
 
 #include "squashfs_fs.h"
 #include "squashfs_fs_sb.h"
@@ -25,6 +26,8 @@ struct squashfs_stream {
 	void		*stream;
 };
 
+static DEFINE_LOCAL_IRQ_LOCK(stream_lock);
+
 void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
 						void *comp_opts)
 {
@@ -79,10 +82,15 @@ int squashfs_decompress(struct squashfs_sb_info *msblk, struct buffer_head **bh,
 {
 	struct squashfs_stream __percpu *percpu =
 			(struct squashfs_stream __percpu *) msblk->stream;
-	struct squashfs_stream *stream = get_cpu_ptr(percpu);
-	int res = msblk->decompressor->decompress(msblk, stream->stream, bh, b,
-		offset, length, output);
-	put_cpu_ptr(stream);
+	struct squashfs_stream *stream;
+	int res;
+
+	stream = get_locked_ptr(stream_lock, percpu);
+
+	res = msblk->decompressor->decompress(msblk, stream->stream, bh, b,
+			offset, length, output);
+
+	put_locked_ptr(stream_lock, stream);
 
 	if (res < 0)
 		ERROR("%s decompression failed, data probably corrupt\n",
diff --git a/include/linux/locallock.h b/include/linux/locallock.h
index d658c2552601..c3ab5183a6a1 100644
--- a/include/linux/locallock.h
+++ b/include/linux/locallock.h
@@ -222,6 +222,14 @@ static inline int __local_unlock_irqrestore(struct local_irq_lock *lv,
 
 #define put_locked_var(lvar, var)	local_unlock(lvar);
 
+#define get_locked_ptr(lvar, var)					\
+	({								\
+		local_lock(lvar);					\
+		this_cpu_ptr(var);					\
+	})
+
+#define put_locked_ptr(lvar, var)	local_unlock(lvar);
+
 #define local_lock_cpu(lvar)						\
 	({								\
 		local_lock(lvar);					\

  reply	other threads:[~2018-05-03 15:47 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-02 13:12 [PATCH 1/1] squashfs: Disable "percpu multiple decompressor" on RT Alexander Stein
2018-05-02 14:37 ` Julia Cartwright
2018-05-03  6:36   ` Alexander Stein
2018-05-03 15:48     ` Julia Cartwright [this message]
2018-05-07  6:09       ` Alexander Stein
2018-05-07 13:58         ` [PATCH RT 1/2] locallock: provide {get,put}_locked_ptr() variants Julia Cartwright
2018-05-07 13:58           ` [PATCH RT 2/2] squashfs: make use of local lock in multi_cpu decompressor Julia Cartwright

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180503154834.GF967@jcartwri.amer.corp.natinst.com \
    --to=julia@ni.com \
    --cc=alexander.stein@systec-electronic.com \
    --cc=bigeasy@linutronix.de \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.