All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Martin Wilck <mwilck@suse.com>
Cc: qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org
Subject: Re: [PATCH v2] virtio-rng: return available data with O_NONBLOCK
Date: Tue, 11 Aug 2020 08:58:53 -0400	[thread overview]
Message-ID: <20200811084616-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <e75b3cd527bae4c6762af17a0a32f57c61191274.camel@suse.com>

On Tue, Aug 11, 2020 at 02:07:23PM +0200, Martin Wilck wrote:
> On Tue, 2020-08-11 at 07:26 -0400, Michael S. Tsirkin wrote:
> > On Wed, Jul 15, 2020 at 03:32:55PM +0200, mwilck@suse.com wrote:
> > >  drivers/char/hw_random/virtio-rng.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/char/hw_random/virtio-rng.c
> > > b/drivers/char/hw_random/virtio-rng.c
> > > index 79a6e47b5fbc..984713b35892 100644
> > > --- a/drivers/char/hw_random/virtio-rng.c
> > > +++ b/drivers/char/hw_random/virtio-rng.c
> > > @@ -59,6 +59,20 @@ static int virtio_read(struct hwrng *rng, void
> > > *buf, size_t size, bool wait)
> > >  	if (vi->hwrng_removed)
> > >  		return -ENODEV;
> > >  
> > > +	/*
> > > +	 * If the previous call was non-blocking, we may have got some
> > > +	 * randomness already.
> > > +	 */
> > > +	if (vi->busy && completion_done(&vi->have_data)) {
> > > +		unsigned int len;
> > > +
> > > +		vi->busy = false;
> > > +		len = vi->data_avail > size ? size : vi->data_avail;
> > > +		vi->data_avail -= len;
> > 
> > I wonder what purpose does this line serve: busy is false
> > which basically means data_avail is invalid, right?
> > A following non blocking call will not enter here.
> 
> Well, I thought this is just how reading data normally works. But
> you're right, the remainder will not be used. I can remove the line, or
> reset data_avail to 0 at this point. What do you prefer?
> 
> Regards,
> Martin

Removing seems cleaner.

But looking at it, it is using the API in a very strange way:
a buffer is placed in the ring by one call, and *assumed*
to still be there in the next call.

which it might not be if one call is from userspace and the
next one is from fill kthread.

I guess this is why it's returning 0: yes it knows there's
data, but it does not know where it is.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Martin Wilck <mwilck@suse.com>
Cc: Jason Wang <jasowang@redhat.com>,
	qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org
Subject: Re: [PATCH v2] virtio-rng: return available data with O_NONBLOCK
Date: Tue, 11 Aug 2020 08:58:53 -0400	[thread overview]
Message-ID: <20200811084616-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <e75b3cd527bae4c6762af17a0a32f57c61191274.camel@suse.com>

On Tue, Aug 11, 2020 at 02:07:23PM +0200, Martin Wilck wrote:
> On Tue, 2020-08-11 at 07:26 -0400, Michael S. Tsirkin wrote:
> > On Wed, Jul 15, 2020 at 03:32:55PM +0200, mwilck@suse.com wrote:
> > >  drivers/char/hw_random/virtio-rng.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/char/hw_random/virtio-rng.c
> > > b/drivers/char/hw_random/virtio-rng.c
> > > index 79a6e47b5fbc..984713b35892 100644
> > > --- a/drivers/char/hw_random/virtio-rng.c
> > > +++ b/drivers/char/hw_random/virtio-rng.c
> > > @@ -59,6 +59,20 @@ static int virtio_read(struct hwrng *rng, void
> > > *buf, size_t size, bool wait)
> > >  	if (vi->hwrng_removed)
> > >  		return -ENODEV;
> > >  
> > > +	/*
> > > +	 * If the previous call was non-blocking, we may have got some
> > > +	 * randomness already.
> > > +	 */
> > > +	if (vi->busy && completion_done(&vi->have_data)) {
> > > +		unsigned int len;
> > > +
> > > +		vi->busy = false;
> > > +		len = vi->data_avail > size ? size : vi->data_avail;
> > > +		vi->data_avail -= len;
> > 
> > I wonder what purpose does this line serve: busy is false
> > which basically means data_avail is invalid, right?
> > A following non blocking call will not enter here.
> 
> Well, I thought this is just how reading data normally works. But
> you're right, the remainder will not be used. I can remove the line, or
> reset data_avail to 0 at this point. What do you prefer?
> 
> Regards,
> Martin

Removing seems cleaner.

But looking at it, it is using the API in a very strange way:
a buffer is placed in the ring by one call, and *assumed*
to still be there in the next call.

which it might not be if one call is from userspace and the
next one is from fill kthread.

I guess this is why it's returning 0: yes it knows there's
data, but it does not know where it is.

-- 
MST



  reply	other threads:[~2020-08-11 12:59 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-15 13:32 [PATCH v2] virtio-rng: return available data with O_NONBLOCK mwilck
2020-08-11 10:23 ` Reminder: " Martin Wilck
2020-08-11 10:37 ` Philippe Mathieu-Daudé
2020-08-11 12:02   ` Laurent Vivier
2020-08-11 12:02     ` Laurent Vivier
2020-08-11 12:22     ` Martin Wilck
2020-08-11 12:39       ` Laurent Vivier
2020-08-11 12:39         ` Laurent Vivier
2020-08-11 12:53         ` Martin Wilck
2020-08-11 13:00           ` Laurent Vivier
2020-08-11 13:00             ` Laurent Vivier
2020-08-11 13:14             ` Michael S. Tsirkin
2020-08-11 13:14               ` Michael S. Tsirkin
2020-08-11 13:53               ` Laurent Vivier
2020-08-11 13:53                 ` Laurent Vivier
2020-08-11 14:49                 ` Michael S. Tsirkin
2020-08-11 14:49                   ` Michael S. Tsirkin
2020-08-11 15:00                   ` Laurent Vivier
2020-08-11 15:00                     ` Laurent Vivier
2020-08-11 14:12           ` Martin Wilck
2020-08-11 11:26 ` Michael S. Tsirkin
2020-08-11 11:26   ` Michael S. Tsirkin
2020-08-11 12:07   ` Martin Wilck
2020-08-11 12:58     ` Michael S. Tsirkin [this message]
2020-08-11 12:58       ` Michael S. Tsirkin

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=20200811084616-mutt-send-email-mst@kernel.org \
    --to=mst@redhat.com \
    --cc=mwilck@suse.com \
    --cc=qemu-devel@nongnu.org \
    --cc=virtualization@lists.linux-foundation.org \
    /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.