Received-SPF: pass (google.com: domain of linux-kernel+bounces-49835-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240127020833.487907-1-kent.overstreet@linux.dev>
 <20240127020833.487907-2-kent.overstreet@linux.dev> <20240202120357.tfjdri5rfd2onajl@quack3>
In-Reply-To: <20240202120357.tfjdri5rfd2onajl@quack3>
Reply-To: sedat.dilek@gmail.com
From: Sedat Dilek <sedat.dilek@gmail.com>
Date: Fri, 2 Feb 2024 13:25:20 +0100
Message-ID: <CA+icZUWdUUhWg_-0NvF+6L=EUhj7amv_7HRKHDPvrEBspwHC2Q@mail.gmail.com>
Subject: Re: [PATCH 1/4] fs/pipe: Convert to lockdep_cmp_fn
To: Jan Kara <jack@suse.cz>
Cc: Kent Overstreet <kent.overstreet@linux.dev>, linux-kernel@vger.kernel.org, 
	netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org, peterz@infradead.org, 
	boqun.feng@gmail.com, Alexander Viro <viro@zeniv.linux.org.uk>, 
	Christian Brauner <brauner@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 2, 2024 at 1:12=E2=80=AFPM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 26-01-24 21:08:28, Kent Overstreet wrote:
> > *_lock_nested() is fundamentally broken; lockdep needs to check lock
> > ordering, but we cannot device a total ordering on an unbounded number
> > of elements with only a few subclasses.
> >
> > the replacement is to define lock ordering with a proper comparison
> > function.
> >
> > fs/pipe.c was already doing everything correctly otherwise, nothing
> > much changes here.
> >
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
>
> I had to digest for a while what this new lockdep lock ordering feature i=
s
> about. I have one pending question - what is the motivation of this
> conversion of pipe code? AFAIU we don't have any problems with lockdep
> annotations on pipe->mutex because there are always only two subclasses?
>
>                                                                 Honza

Hi,

"Numbers talk - Bullshit walks." (Linus Torvalds)

In things of pipes - I normally benchmark like this (example):

root# cat /dev/sdc | pipebench > /dev/null

Do you have numbers for your patch-series?

Thanks.

BG,
-Sedat-

[1] https://packages.debian.org/pipebench

>
> > ---
> >  fs/pipe.c | 81 +++++++++++++++++++++++++------------------------------
> >  1 file changed, 36 insertions(+), 45 deletions(-)
> >
> > diff --git a/fs/pipe.c b/fs/pipe.c
> > index f1adbfe743d4..50c8a8596b52 100644
> > --- a/fs/pipe.c
> > +++ b/fs/pipe.c
> > @@ -76,18 +76,20 @@ static unsigned long pipe_user_pages_soft =3D PIPE_=
DEF_BUFFERS * INR_OPEN_CUR;
> >   * -- Manfred Spraul <manfred@colorfullife.com> 2002-05-09
> >   */
> >
> > -static void pipe_lock_nested(struct pipe_inode_info *pipe, int subclas=
s)
> > +#define cmp_int(l, r)                ((l > r) - (l < r))
> > +
> > +#ifdef CONFIG_PROVE_LOCKING
> > +static int pipe_lock_cmp_fn(const struct lockdep_map *a,
> > +                         const struct lockdep_map *b)
> >  {
> > -     if (pipe->files)
> > -             mutex_lock_nested(&pipe->mutex, subclass);
> > +     return cmp_int((unsigned long) a, (unsigned long) b);
> >  }
> > +#endif
> >
> >  void pipe_lock(struct pipe_inode_info *pipe)
> >  {
> > -     /*
> > -      * pipe_lock() nests non-pipe inode locks (for writing to a file)
> > -      */
> > -     pipe_lock_nested(pipe, I_MUTEX_PARENT);
> > +     if (pipe->files)
> > +             mutex_lock(&pipe->mutex);
> >  }
> >  EXPORT_SYMBOL(pipe_lock);
> >
> > @@ -98,28 +100,16 @@ void pipe_unlock(struct pipe_inode_info *pipe)
> >  }
> >  EXPORT_SYMBOL(pipe_unlock);
> >
> > -static inline void __pipe_lock(struct pipe_inode_info *pipe)
> > -{
> > -     mutex_lock_nested(&pipe->mutex, I_MUTEX_PARENT);
> > -}
> > -
> > -static inline void __pipe_unlock(struct pipe_inode_info *pipe)
> > -{
> > -     mutex_unlock(&pipe->mutex);
> > -}
> > -
> >  void pipe_double_lock(struct pipe_inode_info *pipe1,
> >                     struct pipe_inode_info *pipe2)
> >  {
> >       BUG_ON(pipe1 =3D=3D pipe2);
> >
> > -     if (pipe1 < pipe2) {
> > -             pipe_lock_nested(pipe1, I_MUTEX_PARENT);
> > -             pipe_lock_nested(pipe2, I_MUTEX_CHILD);
> > -     } else {
> > -             pipe_lock_nested(pipe2, I_MUTEX_PARENT);
> > -             pipe_lock_nested(pipe1, I_MUTEX_CHILD);
> > -     }
> > +     if (pipe1 > pipe2)
> > +             swap(pipe1, pipe2);
> > +
> > +     pipe_lock(pipe1);
> > +     pipe_lock(pipe2);
> >  }
> >
> >  static void anon_pipe_buf_release(struct pipe_inode_info *pipe,
> > @@ -271,7 +261,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
> >               return 0;
> >
> >       ret =3D 0;
> > -     __pipe_lock(pipe);
> > +     mutex_lock(&pipe->mutex);
> >
> >       /*
> >        * We only wake up writers if the pipe was full when we started
> > @@ -368,7 +358,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
> >                       ret =3D -EAGAIN;
> >                       break;
> >               }
> > -             __pipe_unlock(pipe);
> > +             mutex_unlock(&pipe->mutex);
> >
> >               /*
> >                * We only get here if we didn't actually read anything.
> > @@ -400,13 +390,13 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to=
)
> >               if (wait_event_interruptible_exclusive(pipe->rd_wait, pip=
e_readable(pipe)) < 0)
> >                       return -ERESTARTSYS;
> >
> > -             __pipe_lock(pipe);
> > +             mutex_lock(&pipe->mutex);
> >               was_full =3D pipe_full(pipe->head, pipe->tail, pipe->max_=
usage);
> >               wake_next_reader =3D true;
> >       }
> >       if (pipe_empty(pipe->head, pipe->tail))
> >               wake_next_reader =3D false;
> > -     __pipe_unlock(pipe);
> > +     mutex_unlock(&pipe->mutex);
> >
> >       if (was_full)
> >               wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT =
| EPOLLWRNORM);
> > @@ -462,7 +452,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *fro=
m)
> >       if (unlikely(total_len =3D=3D 0))
> >               return 0;
> >
> > -     __pipe_lock(pipe);
> > +     mutex_lock(&pipe->mutex);
> >
> >       if (!pipe->readers) {
> >               send_sig(SIGPIPE, current, 0);
> > @@ -582,19 +572,19 @@ pipe_write(struct kiocb *iocb, struct iov_iter *f=
rom)
> >                * after waiting we need to re-check whether the pipe
> >                * become empty while we dropped the lock.
> >                */
> > -             __pipe_unlock(pipe);
> > +             mutex_unlock(&pipe->mutex);
> >               if (was_empty)
> >                       wake_up_interruptible_sync_poll(&pipe->rd_wait, E=
POLLIN | EPOLLRDNORM);
> >               kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
> >               wait_event_interruptible_exclusive(pipe->wr_wait, pipe_wr=
itable(pipe));
> > -             __pipe_lock(pipe);
> > +             mutex_lock(&pipe->mutex);
> >               was_empty =3D pipe_empty(pipe->head, pipe->tail);
> >               wake_next_writer =3D true;
> >       }
> >  out:
> >       if (pipe_full(pipe->head, pipe->tail, pipe->max_usage))
> >               wake_next_writer =3D false;
> > -     __pipe_unlock(pipe);
> > +     mutex_unlock(&pipe->mutex);
> >
> >       /*
> >        * If we do do a wakeup event, we do a 'sync' wakeup, because we
> > @@ -629,7 +619,7 @@ static long pipe_ioctl(struct file *filp, unsigned =
int cmd, unsigned long arg)
> >
> >       switch (cmd) {
> >       case FIONREAD:
> > -             __pipe_lock(pipe);
> > +             mutex_lock(&pipe->mutex);
> >               count =3D 0;
> >               head =3D pipe->head;
> >               tail =3D pipe->tail;
> > @@ -639,16 +629,16 @@ static long pipe_ioctl(struct file *filp, unsigne=
d int cmd, unsigned long arg)
> >                       count +=3D pipe->bufs[tail & mask].len;
> >                       tail++;
> >               }
> > -             __pipe_unlock(pipe);
> > +             mutex_unlock(&pipe->mutex);
> >
> >               return put_user(count, (int __user *)arg);
> >
> >  #ifdef CONFIG_WATCH_QUEUE
> >       case IOC_WATCH_QUEUE_SET_SIZE: {
> >               int ret;
> > -             __pipe_lock(pipe);
> > +             mutex_lock(&pipe->mutex);
> >               ret =3D watch_queue_set_size(pipe, arg);
> > -             __pipe_unlock(pipe);
> > +             mutex_unlock(&pipe->mutex);
> >               return ret;
> >       }
> >
> > @@ -734,7 +724,7 @@ pipe_release(struct inode *inode, struct file *file=
)
> >  {
> >       struct pipe_inode_info *pipe =3D file->private_data;
> >
> > -     __pipe_lock(pipe);
> > +     mutex_lock(&pipe->mutex);
> >       if (file->f_mode & FMODE_READ)
> >               pipe->readers--;
> >       if (file->f_mode & FMODE_WRITE)
> > @@ -747,7 +737,7 @@ pipe_release(struct inode *inode, struct file *file=
)
> >               kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
> >               kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
> >       }
> > -     __pipe_unlock(pipe);
> > +     mutex_unlock(&pipe->mutex);
> >
> >       put_pipe_info(inode, pipe);
> >       return 0;
> > @@ -759,7 +749,7 @@ pipe_fasync(int fd, struct file *filp, int on)
> >       struct pipe_inode_info *pipe =3D filp->private_data;
> >       int retval =3D 0;
> >
> > -     __pipe_lock(pipe);
> > +     mutex_lock(&pipe->mutex);
> >       if (filp->f_mode & FMODE_READ)
> >               retval =3D fasync_helper(fd, filp, on, &pipe->fasync_read=
ers);
> >       if ((filp->f_mode & FMODE_WRITE) && retval >=3D 0) {
> > @@ -768,7 +758,7 @@ pipe_fasync(int fd, struct file *filp, int on)
> >                       /* this can happen only if on =3D=3D T */
> >                       fasync_helper(-1, filp, 0, &pipe->fasync_readers)=
;
> >       }
> > -     __pipe_unlock(pipe);
> > +     mutex_unlock(&pipe->mutex);
> >       return retval;
> >  }
> >
> > @@ -834,6 +824,7 @@ struct pipe_inode_info *alloc_pipe_info(void)
> >               pipe->nr_accounted =3D pipe_bufs;
> >               pipe->user =3D user;
> >               mutex_init(&pipe->mutex);
> > +             lock_set_cmp_fn(&pipe->mutex, pipe_lock_cmp_fn, NULL);
> >               return pipe;
> >       }
> >
> > @@ -1144,7 +1135,7 @@ static int fifo_open(struct inode *inode, struct =
file *filp)
> >       filp->private_data =3D pipe;
> >       /* OK, we have a pipe and it's pinned down */
> >
> > -     __pipe_lock(pipe);
> > +     mutex_lock(&pipe->mutex);
> >
> >       /* We can only do regular read/write on fifos */
> >       stream_open(inode, filp);
> > @@ -1214,7 +1205,7 @@ static int fifo_open(struct inode *inode, struct =
file *filp)
> >       }
> >
> >       /* Ok! */
> > -     __pipe_unlock(pipe);
> > +     mutex_unlock(&pipe->mutex);
> >       return 0;
> >
> >  err_rd:
> > @@ -1230,7 +1221,7 @@ static int fifo_open(struct inode *inode, struct =
file *filp)
> >       goto err;
> >
> >  err:
> > -     __pipe_unlock(pipe);
> > +     mutex_unlock(&pipe->mutex);
> >
> >       put_pipe_info(inode, pipe);
> >       return ret;
> > @@ -1411,7 +1402,7 @@ long pipe_fcntl(struct file *file, unsigned int c=
md, unsigned int arg)
> >       if (!pipe)
> >               return -EBADF;
> >
> > -     __pipe_lock(pipe);
> > +     mutex_lock(&pipe->mutex);
> >
> >       switch (cmd) {
> >       case F_SETPIPE_SZ:
> > @@ -1425,7 +1416,7 @@ long pipe_fcntl(struct file *file, unsigned int c=
md, unsigned int arg)
> >               break;
> >       }
> >
> > -     __pipe_unlock(pipe);
> > +     mutex_unlock(&pipe->mutex);
> >       return ret;
> >  }
> >
> > --
> > 2.43.0
> >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>