Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1739296 > unrolled thread
| Started by | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| First post | 2017-09-26 00:10 +0200 |
| Last post | 2017-09-27 08:40 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers. Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-09-26 00:10 +0200
Re: [PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers. Pankaj Gupta <pagupta@redhat.com> - 2017-09-26 08:40 +0200
Re: [PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers. Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-09-26 19:00 +0200
Re: [PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers. Pankaj Gupta <pagupta@redhat.com> - 2017-09-27 08:40 +0200
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2017-09-26 00:10 +0200 |
| Subject | Re: [PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers. |
| Message-ID | <utJJD-43z-7@gated-at.bofh.it> |
A bit late to a party, but:
On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong <akong@redhat.com> wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> There's currently a big lock around everything, and it means that we
> can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> while the rng is reading. This is a real problem when the rng is slow,
> or blocked (eg. virtio_rng with qemu's default /dev/random backend)
>
> This doesn't help (it leaves the current lock untouched), just adds a
> lock to protect the read function and the static buffers, in preparation
> for transition.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
...
>
> @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf,
> goto out_unlock;
> }
>
> + mutex_lock(&reading_mutex);
I think this breaks O_NONBLOCK: we have hwrng core thread that is
constantly pumps underlying rng for data; the thread takes the mutex
and calls rng_get_data() that blocks until RNG responds. This means
that even user specified O_NONBLOCK here we'll be waiting until
[hwrng] thread releases reading_mutex before we can continue.
> if (!data_avail) {
> bytes_read = rng_get_data(current_rng, rng_buffer,
> rng_buffer_size(),
> !(filp->f_flags & O_NONBLOCK));
> if (bytes_read < 0) {
> err = bytes_read;
> - goto out_unlock;
> + goto out_unlock_reading;
> }
> data_avail = bytes_read;
> }
Thanks.
--
Dmitry
[toc] | [next] | [standalone]
| From | Pankaj Gupta <pagupta@redhat.com> |
|---|---|
| Date | 2017-09-26 08:40 +0200 |
| Message-ID | <utRHd-10R-25@gated-at.bofh.it> |
| In reply to | #1739296 |
>
> A bit late to a party, but:
>
> On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong <akong@redhat.com> wrote:
> > From: Rusty Russell <rusty@rustcorp.com.au>
> >
> > There's currently a big lock around everything, and it means that we
> > can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> > while the rng is reading. This is a real problem when the rng is slow,
> > or blocked (eg. virtio_rng with qemu's default /dev/random backend)
> >
> > This doesn't help (it leaves the current lock untouched), just adds a
> > lock to protect the read function and the static buffers, in preparation
> > for transition.
> >
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > ---
> ...
> >
> > @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char
> > __user *buf,
> > goto out_unlock;
> > }
> >
> > + mutex_lock(&reading_mutex);
>
> I think this breaks O_NONBLOCK: we have hwrng core thread that is
> constantly pumps underlying rng for data; the thread takes the mutex
> and calls rng_get_data() that blocks until RNG responds. This means
> that even user specified O_NONBLOCK here we'll be waiting until
> [hwrng] thread releases reading_mutex before we can continue.
I think for 'virtio_rng' for 'O_NON_BLOCK' 'rng_get_data' returns
without waiting for data which can let mutex to be used by other
threads waiting if any?
rng_dev_read
rng_get_data
virtio_read
static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
{
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
if (vi->hwrng_removed)
return -ENODEV;
if (!vi->busy) {
vi->busy = true;
init_completion(&vi->have_data);
register_buffer(vi, buf, size);
}
if (!wait)
return 0;
ret = wait_for_completion_killable(&vi->have_data);
if (ret < 0)
return ret;
vi->busy = false;
return vi->data_avail;
}
>
> > if (!data_avail) {
> > bytes_read = rng_get_data(current_rng, rng_buffer,
> > rng_buffer_size(),
> > !(filp->f_flags & O_NONBLOCK));
> > if (bytes_read < 0) {
> > err = bytes_read;
> > - goto out_unlock;
> > + goto out_unlock_reading;
> > }
> > data_avail = bytes_read;
> > }
>
> Thanks.
>
> --
> Dmitry
>
[toc] | [prev] | [next] | [standalone]
| From | Dmitry Torokhov <dmitry.torokhov@gmail.com> |
|---|---|
| Date | 2017-09-26 19:00 +0200 |
| Message-ID | <uu1nb-7hm-3@gated-at.bofh.it> |
| In reply to | #1739548 |
On Tue, Sep 26, 2017 at 02:36:57AM -0400, Pankaj Gupta wrote: > > > > > A bit late to a party, but: > > > > On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong <akong@redhat.com> wrote: > > > From: Rusty Russell <rusty@rustcorp.com.au> > > > > > > There's currently a big lock around everything, and it means that we > > > can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current) > > > while the rng is reading. This is a real problem when the rng is slow, > > > or blocked (eg. virtio_rng with qemu's default /dev/random backend) > > > > > > This doesn't help (it leaves the current lock untouched), just adds a > > > lock to protect the read function and the static buffers, in preparation > > > for transition. > > > > > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > > --- > > ... > > > > > > @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char > > > __user *buf, > > > goto out_unlock; > > > } > > > > > > + mutex_lock(&reading_mutex); > > > > I think this breaks O_NONBLOCK: we have hwrng core thread that is > > constantly pumps underlying rng for data; the thread takes the mutex > > and calls rng_get_data() that blocks until RNG responds. This means > > that even user specified O_NONBLOCK here we'll be waiting until > > [hwrng] thread releases reading_mutex before we can continue. > > I think for 'virtio_rng' for 'O_NON_BLOCK' 'rng_get_data' returns > without waiting for data which can let mutex to be used by other > threads waiting if any? > > rng_dev_read > rng_get_data > virtio_read As I said in the paragraph above the code that potentially holds the mutex for long time is the thread in hwrng core: hwrng_fillfn(). As it calls rng_get_data() with "wait" argument == 1 it may block while holding reading_mutex, which, in turn, will block rng_dev_read(), even if it was called with O_NONBLOCK. Thanks. -- Dmitry
[toc] | [prev] | [next] | [standalone]
| From | Pankaj Gupta <pagupta@redhat.com> |
|---|---|
| Date | 2017-09-27 08:40 +0200 |
| Message-ID | <uueaK-79N-7@gated-at.bofh.it> |
| In reply to | #1740036 |
>
> On Tue, Sep 26, 2017 at 02:36:57AM -0400, Pankaj Gupta wrote:
> >
> > >
> > > A bit late to a party, but:
> > >
> > > On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong <akong@redhat.com> wrote:
> > > > From: Rusty Russell <rusty@rustcorp.com.au>
> > > >
> > > > There's currently a big lock around everything, and it means that we
> > > > can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> > > > while the rng is reading. This is a real problem when the rng is slow,
> > > > or blocked (eg. virtio_rng with qemu's default /dev/random backend)
> > > >
> > > > This doesn't help (it leaves the current lock untouched), just adds a
> > > > lock to protect the read function and the static buffers, in
> > > > preparation
> > > > for transition.
> > > >
> > > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > > > ---
> > > ...
> > > >
> > > > @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp,
> > > > char
> > > > __user *buf,
> > > > goto out_unlock;
> > > > }
> > > >
> > > > + mutex_lock(&reading_mutex);
> > >
> > > I think this breaks O_NONBLOCK: we have hwrng core thread that is
> > > constantly pumps underlying rng for data; the thread takes the mutex
> > > and calls rng_get_data() that blocks until RNG responds. This means
> > > that even user specified O_NONBLOCK here we'll be waiting until
> > > [hwrng] thread releases reading_mutex before we can continue.
> >
> > I think for 'virtio_rng' for 'O_NON_BLOCK' 'rng_get_data' returns
> > without waiting for data which can let mutex to be used by other
> > threads waiting if any?
> >
> > rng_dev_read
> > rng_get_data
> > virtio_read
>
> As I said in the paragraph above the code that potentially holds the
> mutex for long time is the thread in hwrng core: hwrng_fillfn(). As it
> calls rng_get_data() with "wait" argument == 1 it may block while
> holding reading_mutex, which, in turn, will block rng_dev_read(), even
> if it was called with O_NONBLOCK.
yes, 'hwrng_fillfn' does not consider O_NONBLOCK and can result in mutex wait
for other tasks. What if we pass zero for wait to 'hwrng_fill' to return early
if there is no data?
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -403,7 +403,7 @@ static int hwrng_fillfn(void *unused)
break;
mutex_lock(&reading_mutex);
rc = rng_get_data(rng, rng_fillbuf,
- rng_buffer_size(), 1);
+ rng_buffer_size(), 0);
mutex_unlock(&reading_mutex);
put_rng(rng);
if (rc <= 0) {
Thanks,
Pankaj
>
> Thanks.
>
> --
> Dmitry
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web