From owner-svn-src-all@FreeBSD.ORG Sun Nov 2 21:39:13 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E4754521; Sun, 2 Nov 2014 21:39:13 +0000 (UTC) Received: from anubis.delphij.net (anubis.delphij.net [IPv6:2001:470:1:117::25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "anubis.delphij.net", Issuer "StartCom Class 1 Primary Intermediate Server CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id BAAD893A; Sun, 2 Nov 2014 21:39:13 +0000 (UTC) Received: from zeta.ixsystems.com (c-24-5-244-32.hsd1.ca.comcast.net [24.5.244.32]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by anubis.delphij.net (Postfix) with ESMTPSA id 8CFA514DDE; Sun, 2 Nov 2014 13:39:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=delphij.net; s=anubis; t=1414964353; x=1414978753; bh=/cyntB4lnWXQ9sD5mO1P9G3BsIK/mHBQcLGuriUtMYw=; h=Date:From:Reply-To:To:CC:Subject:References:In-Reply-To; b=U8Tgwd9S6YLWZ1z7I9YgACy7g33Hxq5B8Fudd0LfB8jQANNyVM1SQkloO/luwpd7r Ak4hQPFNHbCR1F3OsXaYoeDVsshzKb6wXHZfN8gUzLlxz0ZMZdMYyGXtMhb5WRprII NM4yMzKjK2kEanyqkMTXNIplJFSr7/f4YQiFX1B0= Message-ID: <5456A47E.2030405@delphij.net> Date: Sun, 02 Nov 2014 13:39:10 -0800 From: Xin Li Reply-To: d@delphij.net Organization: The FreeBSD Project MIME-Version: 1.0 To: Ian Lepore , d@delphij.net Subject: Re: svn commit: r273958 - head/sys/dev/random References: <201411020201.sA221unt091493@svn.freebsd.org> <720EB74E-094A-43F3-8B1C-47BC7F6FECC3@grondar.org> <1414934579.17308.248.camel@revolution.hippie.lan> <6FB65828-6A79-4BDE-A9F7-BC472BA538CE@grondar.org> <20141102192057.GB53947@kib.kiev.ua> <29A795E1-19E2-49E4-9653-143D3F6F3F12@grondar.org> <20141102194625.GC53947@kib.kiev.ua> <751CD860-95B9-4F68-AE69-976B42823AD0@grondar.org> <54568E41.8030305@delphij.net> <20141102201331.GE53947@kib.kiev.ua> <545693B4.8030602@delphij.net> <1414961583.1200.27.camel@revolution.hippie.lan> In-Reply-To: <1414961583.1200.27.camel@revolution.hippie.lan> Content-Type: multipart/mixed; boundary="------------090504050004050003030801" Cc: Konstantin Belousov , "svn-src-head@freebsd.org" , "svn-src-all@freebsd.org" , "src-committers@freebsd.org" , Mark R V Murray X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 02 Nov 2014 21:39:14 -0000 This is a multi-part message in MIME format. --------------090504050004050003030801 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 11/02/14 12:53, Ian Lepore wrote: > On Sun, 2014-11-02 at 12:27 -0800, Xin Li wrote: >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 >> >> Hi, Mark, >> >> I'd like to propose the attached patch for review. It replaces >> tsleep's with sx_sleep's, then checks the return value and quit >> the loop. >> >> Cheers, - -- > > It still doesn't handle the partial read/write case Kostik > mentioned, but there are plenty of other drivers that don't get > that right. Given that the ra_read/ra_write functions can't return > error, it would only be errors from uiomove() in play. I guess it > would be something like this: > > nbytes = uio->uio_resid; while (uio->uio_resid && !error) { c = > MIN(uio->uio_resid, PAGE_SIZE); > (random_adaptor->ra_read)(random_buf, c); error = > uiomove(random_buf, c, uio); } if (uio->uio_resid != nbytes) error > = 0; /* Return partial read, not error. */ > > Also, there's now a mix of if (error == 0) and if (!error) near > each other (I tend to prefer using ! only on boolean_t, but I even > more prefer local consistancy. :) Thanks for pointing this out. I've added some checks at: http://pastebin.com/ZGsqpUHR Also attached. Cheers, - -- Xin LI https://www.delphij.net/ FreeBSD - The Power to Serve! Live free or die -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0 iQIcBAEBCgAGBQJUVqR+AAoJEJW2GBstM+nsLl8QAIFQQZCGwwkeTlVkG/i9YzAl pK4uox2dXouN1Zfwi9Q3ymB0DsUbnXt9IEP9JCD4Z8ZeFKUIC2mNI/V+t3etV5Kr kVdvayizwbJ0jLBYu6rUIoeZeqWFnLB25PitJFCPOAqJ+H1h/hV56Zx9SvkAWEIe spGpvq+eeHRYl/AzVMQFHjax0V7bo3yd+3klDlU3DT77WFvSRngNKUxHgVPMsoBY xm4YRPy6MLAHXlEoHtoERc5Qyhj00Km4ZRHtXn3tFh6LSAdtXmbBiVXlHWZ0eVEn 5vH0d3MKRCnKtQg2ydEA5SWoCTMHIVCfoB/yQ4B0N0FupgdEUXMlag0iBV8DZcmb nv1sd68V3GgDjiiAwevhbSLQ0cHr5FABtTdXwExClR2sOh3rN1PONsfbzfs3FgkO TWts6znUcblmpCFyQyz49/r+SQ35S1YXCKIZB5Z43yysPgR20uuSKbD0XKAne28u acntrXXgrfERSue1T5qz2xwH+dBWGuJtGDq1wSeP1kw8aWoX+ApAbUColCY2pEhm OawGlJ6qnr4VgEIl4vQ/S9gaxMX33K/KCHJERaCA/8h1WQQklmRTky4URZ6bAaHJ NMEd6GYD25GmfIY7kHB/tuPfjiWDwGnNh8mvSSAZBJM81gW+/9xqKwKn3OZ3kMFY eM+SpnlDm4skN3geKGse =yQhY -----END PGP SIGNATURE----- --------------090504050004050003030801 Content-Type: text/plain; charset=UTF-8; name="random-tsleep.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="random-tsleep.diff" Index: sys/dev/random/random_adaptors.c =================================================================== --- sys/dev/random/random_adaptors.c (revision 273982) +++ sys/dev/random/random_adaptors.c (working copy) @@ -171,9 +171,8 @@ random_adaptor_register(const char *name, struct r sx_xlock(&random_adaptors_lock); LIST_INSERT_HEAD(&random_adaptors_list, rra, rra_entries); random_adaptor_choose(); + KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); sx_xunlock(&random_adaptors_lock); - - KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); } void @@ -182,9 +181,9 @@ random_adaptor_deregister(const char *name) struct random_adaptors *rra; KASSERT(name != NULL, ("invalid input to %s", __func__)); - KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); sx_xlock(&random_adaptors_lock); + KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); LIST_FOREACH(rra, &random_adaptors_list, rra_entries) if (strcmp(rra->rra_name, name) == 0) { LIST_REMOVE(rra, rra_entries); @@ -208,16 +207,18 @@ random_adaptor_read(struct cdev *dev __unused, str printf("random: %s %ld\n", __func__, uio->uio_resid); #endif - KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); + random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK); sx_slock(&random_adaptors_lock); + KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); + /* Let the entropy source do any pre-read setup. */ (random_adaptor->ra_read)(NULL, 0); /* (Un)Blocking logic */ error = 0; - while (!random_adaptor->ra_seeded()) { + while (!random_adaptor->ra_seeded() && error == 0) { if (flags & O_NONBLOCK) { error = EWOULDBLOCK; break; @@ -224,7 +225,10 @@ random_adaptor_read(struct cdev *dev __unused, str } /* Sleep instead of going into a spin-frenzy */ - tsleep(&random_adaptor, PUSER | PCATCH, "block", hz/10); + error = sx_sleep(&random_adaptor, &random_adaptors_lock, + PUSER | PCATCH, "block", hz/10); + KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", + __func__)); /* keep tapping away at the pre-read until we seed/unblock. */ (random_adaptor->ra_read)(NULL, 0); @@ -241,12 +245,10 @@ random_adaptor_read(struct cdev *dev __unused, str mtx_unlock(&random_read_rate_mtx); - if (!error) { + if (error == 0) { + nbytes = uio->uio_resid; /* The actual read */ - - random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK); - while (uio->uio_resid && !error) { c = MIN(uio->uio_resid, PAGE_SIZE); (random_adaptor->ra_read)(random_buf, c); @@ -256,11 +258,15 @@ random_adaptor_read(struct cdev *dev __unused, str /* Let the entropy source do any post-read cleanup. */ (random_adaptor->ra_read)(NULL, 1); - free(random_buf, M_ENTROPY); + if (nbytes != uio->uio_resid && (error == ERESTART || + error == EINTR) ) + error = 0; /* Return partial read, not error. */ + } - sx_sunlock(&random_adaptors_lock); + free(random_buf, M_ENTROPY); + return (error); } @@ -269,6 +275,8 @@ random_adaptor_read_rate(void) { int ret; + sx_assert(&random_adaptors_lock, SA_LOCKED); + KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); mtx_lock(&random_read_rate_mtx); @@ -287,18 +295,20 @@ random_adaptor_write(struct cdev *dev __unused, st { int c, error = 0; void *random_buf; + ssize_t nbytes; #ifdef RANDOM_DEBUG printf("random: %s %zd\n", __func__, uio->uio_resid); #endif - KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); + random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK); sx_slock(&random_adaptors_lock); - random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK); + KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); - while (uio->uio_resid > 0) { + nbytes = uio->uio_resid; + while (uio->uio_resid > 0 && error == 0) { c = MIN(uio->uio_resid, PAGE_SIZE); error = uiomove(random_buf, c, uio); if (error) @@ -306,13 +316,20 @@ random_adaptor_write(struct cdev *dev __unused, st (random_adaptor->ra_write)(random_buf, c); /* Introduce an annoying delay to stop swamping */ - tsleep(&random_adaptor, PUSER | PCATCH, "block", hz/10); + error = sx_sleep(&random_adaptor, &random_adaptors_lock, + PUSER | PCATCH, "block", hz/10); + KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", + __func__)); } + sx_sunlock(&random_adaptors_lock); + + if (nbytes != uio->uio_resid && (error == ERESTART || + error == EINTR) ) + error = 0; /* Partial write, not error. */ + free(random_buf, M_ENTROPY); - sx_sunlock(&random_adaptors_lock); - return (error); } @@ -325,10 +342,10 @@ random_adaptor_poll(struct cdev *dev __unused, int printf("random: %s\n", __func__); #endif + sx_slock(&random_adaptors_lock); + KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); - sx_slock(&random_adaptors_lock); - if (events & (POLLIN | POLLRDNORM)) { if (random_adaptor->ra_seeded()) events &= (POLLIN | POLLRDNORM); @@ -382,9 +399,9 @@ random_sysctl_active_adaptor_handler(SYSCTL_HANDLE struct sbuf sbuf; int error; + sx_slock(&random_adaptors_lock); KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); - sx_slock(&random_adaptors_lock); sbuf_new_for_sysctl(&sbuf, NULL, 16, req); LIST_FOREACH(rra, &random_adaptors_list, rra_entries) if (rra->rra_ra == random_adaptor) { @@ -454,9 +471,9 @@ static void random_adaptors_seed(void *unused __unused) { + sx_slock(&random_adaptors_lock); KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__)); - sx_slock(&random_adaptors_lock); random_adaptor->ra_reseed(); sx_sunlock(&random_adaptors_lock); --------------090504050004050003030801 Content-Type: application/octet-stream; name="random-tsleep.diff.sig" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="random-tsleep.diff.sig" iQIcBAABCgAGBQJUVqR+AAoJEJW2GBstM+ns37wP/iTTXi8vOQwX3a/Rbs74kml917+RlNMI sTzCEiymLTw1eCF458oGAbKx/F03Te964FvaAaL5gvyme39fSon35AnBPwjbgv8gABWjoElQ 1KshqtjLsvrSGyF7ev15dXTugw/J1zRW6d9E6qxE2qU6kLXh8KO3Al8AyRSmOEArB/tvhkyW yu96o9f8pDQjFsrv4uaMg4bMxkbvBg5F0Yo5jNFmihWZDngWykcybTs4+mxP4fKBX8fCfVRJ 0MNXDP6BO/nbzTagMMQaIh3ADwIeccsQDsAbGsfYtuH36cY7FzMIsVske0VvXkAPTUPHi8sc WwlzwsI1jn/zZZHHn8IMR5QqahGmE/wt0lLUNqMRD0le2pZ24xoR6W2DxCBKrwsJOq0p9gkI rjXOyUj1fb5v43+s/+TnrShP0ja+B2MSvYGZigOnO9ykMmxWME4VYBnA5NYPtNlcioJDSAU+ 9JhXvptxW1BGhASsHgaSYk6KbnINHjd3G0z6E3qrSfBsA0bm7j928gQnnlYKTMch4gq3PIYi lGAYnTY/7V6b49ArZHlihPaywxTGhg1b9SLnzUgozhsiYf2WvMta+ORFs1fcqd1hrESqzoJ0 rQjQcbcfZqe/R8kcv7u4cSOFq0SD0r/a+/QG2XVhWiFz1Bg+UT4drX3TbQJe6BJe7ht2CJ5Q VOAP --------------090504050004050003030801--