| Message ID | 1410796949-2221-3-git-send-email-akong@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Amos Kong <akong@redhat.com> writes: > I started a QEMU (non-smp) guest with one virtio-rng device, and read > random data from /dev/hwrng by dd: > > # dd if=/dev/hwrng of=/dev/null & > > In the same time, if I check hwrng attributes from sysfs by cat: > > # cat /sys/class/misc/hw_random/rng_* > > The cat process always gets stuck with slow backend (5 k/s), if we > use a quick backend (1.2 M/s), the cat process will cost 1 to 2 > minutes. The stuck doesn't exist for smp guest. > > Reading syscall enters kernel and call rng_dev_read(), it's user > context. We used need_resched() to check if other tasks need to > be run, but it almost always return false, and re-hold the mutex > lock. The attributes accessing process always fails to hold the > lock, so the cat gets stuck. > > User context doesn't allow other user contexts run on that CPU, > unless the kernel code sleeps for some reason. This is why the > need_reshed() always return false here. > > This patch removed need_resched() and always schedule other tasks > then other tasks can have chance to hold the lock and execute > protected code. OK, this is going to be a rant. Your explanation doesn't make sense at all. Worse, your solution breaks the advice of Kernighan & Plaugher: "Don't patch bad code - rewrite it.". But worst of all, this detailed explanation might have convinced me you understood the problem better than I did, and applied your patch. I did some tests. For me, as expected, the process spends its time inside the virtio rng read function, holding the mutex and thus blocking sysfs access; it's not a failure of this code at all. Your schedule_timeout() "fix" probably just helps by letting the host refresh entropy, so we spend less time waiting in the read fn. I will post a series, which unfortunately is only lightly tested, then I'm going to have some beer to begin my holiday. That may help me forget my disappointment at seeing respected fellow developers monkey-patching random code they don't understand. Grrr.... Rusty. > Signed-off-by: Amos Kong <akong@redhat.com> > --- > drivers/char/hw_random/core.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > index c591d7e..263a370 100644 > --- a/drivers/char/hw_random/core.c > +++ b/drivers/char/hw_random/core.c > @@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, > > mutex_unlock(&rng_mutex); > > - if (need_resched()) > - schedule_timeout_interruptible(1); > + schedule_timeout_interruptible(1); > > if (signal_pending(current)) { > err = -ERESTARTSYS; > -- > 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 18, 2014 at 12:13:08PM +0930, Rusty Russell wrote: > Amos Kong <akong@redhat.com> writes: > > > I started a QEMU (non-smp) guest with one virtio-rng device, and read > > random data from /dev/hwrng by dd: > > > > # dd if=/dev/hwrng of=/dev/null & > > > > In the same time, if I check hwrng attributes from sysfs by cat: > > > > # cat /sys/class/misc/hw_random/rng_* > > > > The cat process always gets stuck with slow backend (5 k/s), if we > > use a quick backend (1.2 M/s), the cat process will cost 1 to 2 > > minutes. The stuck doesn't exist for smp guest. > > > > Reading syscall enters kernel and call rng_dev_read(), it's user > > context. We used need_resched() to check if other tasks need to > > be run, but it almost always return false, and re-hold the mutex > > lock. The attributes accessing process always fails to hold the > > lock, so the cat gets stuck. > > > > User context doesn't allow other user contexts run on that CPU, > > unless the kernel code sleeps for some reason. This is why the > > need_reshed() always return false here. > > > > This patch removed need_resched() and always schedule other tasks > > then other tasks can have chance to hold the lock and execute > > protected code. Hi Rusty, > OK, this is going to be a rant. > > Your explanation doesn't make sense at all. Worse, your solution breaks > the advice of Kernighan & Plaugher: "Don't patch bad code - rewrite > it.". > > But worst of all, this detailed explanation might have convinced me you > understood the problem better than I did, and applied your patch. I'm sorry about the misleading. > I did some tests. For me, as expected, the process spends its time > inside the virtio rng read function, holding the mutex and thus blocking > sysfs access; it's not a failure of this code at all. Got it now. The catting hang bug was found when I try to fix unhotplug issue, the unhotplug issue can't be reproduced if I try to debug by gdb or printk. So I forgot to debug cat hang ... but spend time to misunderstand schedle code :( > Your schedule_timeout() "fix" probably just helps by letting the host > refresh entropy, so we spend less time waiting in the read fn. > > I will post a series, which unfortunately is only lightly tested, then > I'm going to have some beer to begin my holiday. That may help me > forget my disappointment at seeing respected fellow developers > monkey-patching random code they don't understand. I just posted a V2 with two additional fixes, hotunplugging works well now :) > Grrr.... Enjoy your holiday! Amos > Rusty. > > > Signed-off-by: Amos Kong <akong@redhat.com> > > --- > > drivers/char/hw_random/core.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > > index c591d7e..263a370 100644 > > --- a/drivers/char/hw_random/core.c > > +++ b/drivers/char/hw_random/core.c > > @@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, > > > > mutex_unlock(&rng_mutex); > > > > - if (need_resched()) > > - schedule_timeout_interruptible(1); > > + schedule_timeout_interruptible(1); > > > > if (signal_pending(current)) { > > err = -ERESTARTSYS; > > -- > > 1.9.3
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index c591d7e..263a370 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, mutex_unlock(&rng_mutex); - if (need_resched()) - schedule_timeout_interruptible(1); + schedule_timeout_interruptible(1); if (signal_pending(current)) { err = -ERESTARTSYS;
I started a QEMU (non-smp) guest with one virtio-rng device, and read random data from /dev/hwrng by dd: # dd if=/dev/hwrng of=/dev/null & In the same time, if I check hwrng attributes from sysfs by cat: # cat /sys/class/misc/hw_random/rng_* The cat process always gets stuck with slow backend (5 k/s), if we use a quick backend (1.2 M/s), the cat process will cost 1 to 2 minutes. The stuck doesn't exist for smp guest. Reading syscall enters kernel and call rng_dev_read(), it's user context. We used need_resched() to check if other tasks need to be run, but it almost always return false, and re-hold the mutex lock. The attributes accessing process always fails to hold the lock, so the cat gets stuck. User context doesn't allow other user contexts run on that CPU, unless the kernel code sleeps for some reason. This is why the need_reshed() always return false here. This patch removed need_resched() and always schedule other tasks then other tasks can have chance to hold the lock and execute protected code. Signed-off-by: Amos Kong <akong@redhat.com> --- drivers/char/hw_random/core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)