From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kay Sievers Date: Wed, 02 May 2007 23:51:34 +0000 Subject: Re: udevd on very large systems: again Message-Id: <1178149894.4346.33.camel@lov.localdomain> MIME-Version: 1 Content-Type: multipart/mixed; boundary="=-9uVwBauO7C6cQUFF2372" List-Id: References: <4637D051.7060103@sgi.com> In-Reply-To: <4637D051.7060103@sgi.com> To: linux-hotplug@vger.kernel.org --=-9uVwBauO7C6cQUFF2372 Content-Type: text/plain Content-Transfer-Encoding: 7bit On Thu, 2007-05-03 at 01:42 +0200, Kay Sievers wrote: > On Wed, 2007-05-02 at 17:26 -0400, George Beshers wrote: > > Kay Sievers wrote: > > > Looks fine so far, but I still don't see how reading a seq-proc-file > > > could miss entries. Are you expecting this to happen? Did you ever see > > > this for any of these files? > > > The answer is slightly complex. No I never saw it happen, but I am > > concerned about it. To illustrate that, consider the following test > > program and the output log attached. Basically, the /proc/stat file > > does seem to be changing dynamically and an open file does not > > present a stable snapshot. > > Looking at the seq_file code, calling lseek() causes the buffer to be > repopulated: > seq_lseek() -> traverse() -> m->op->show() > while show() is the method of the "stat" file that fills the buffer. > > A simple read() should not do that, and just copy the content (up to > 128kB) of the kernel seq_file file buffer to the user-process: > seq_read() -> copy_to_user(buf, m->buf + m->from, n) > > > So the danger is that the procs_running > > might get split at a 4k boundary and the split changes by the time > > the next read occurs so procs_running is corrupted. > > I'm pretty sure, that this doesn't happen, if you don't seek(). Here is > a test, that opens /proc/stat, reads a single byte (which fills the > buffer), forks a ton of processes and reads the rest of the buffer. You > see that the "processes" value did not increase: > $ ./seq > processes 57425 > processes 57425 > processes 58203 The patch should be as simple as this now, I hope. :) Thanks, Kay --=-9uVwBauO7C6cQUFF2372 Content-Disposition: inline; filename=udev-proc-buffer.patch Content-Type: text/x-patch; name=udev-proc-buffer.patch; charset=utf-8 Content-Transfer-Encoding: 7bit diff --git a/udevd.c b/udevd.c index 645b068..4086b96 100644 --- a/udevd.c +++ b/udevd.c @@ -318,61 +318,42 @@ static void msg_queue_insert(struct udev static int mem_size_mb(void) { - int f; - char buf[8192]; - long int len; - const char *pos; - long int memsize; - - f = open("/proc/meminfo", O_RDONLY); - if (f == -1) - return -1; - - len = read(f, buf, sizeof(buf)-1); - close(f); + FILE* f; + char buf[1024]; + long int memsize = -1; - if (len <= 0) + f = fopen("/proc/meminfo", "r"); + if (f == NULL) return -1; - buf[len] = '\0'; - pos = strstr(buf, "MemTotal: "); - if (pos == NULL) - return -1; + while (fgets(buf, sizeof(buf), f) != NULL) { + long int value; - if (sscanf(pos, "MemTotal: %ld kB", &memsize) != 1) - return -1; + if (sscanf(buf, "MemTotal: %ld kB", &value) == 1) { + memsize = value / 1024; + break; + } + } - return memsize / 1024; + return memsize; } static int cpu_count(void) { - int f; - char buf[65536]; - int len; - const char *pos; + FILE* f; + char buf[1024]; int count = 0; - f = open("/proc/stat", O_RDONLY); - if (f == -1) + f = fopen("/proc/stat", "r"); + if (f == NULL) return -1; - len = read(f, buf, sizeof(buf)-1); - close(f); - if (len <= 0) - return -1; - buf[len] = '\0'; - - pos = strstr(buf, "cpu"); - if (pos == NULL) - return -1; - - while (pos != NULL) { - if (strncmp(pos, "cpu", 3) == 0 &&isdigit(pos[3])) + while (fgets(buf, sizeof(buf), f) != NULL) { + if (strncmp(buf, "cpu", 3) == 0 && isdigit(buf[3])) count++; - pos = strstr(&pos[3], "cpu"); } + fclose(f); if (count == 0) return -1; return count; @@ -380,29 +361,24 @@ static int cpu_count(void) static int running_processes(void) { - int f; - char buf[32768]; - int len; - int running; - const char *pos; + FILE* f; + char buf[1024]; + int running = -1; - f = open("/proc/stat", O_RDONLY); - if (f == -1) + f = fopen("/proc/stat", "r"); + if (f == NULL) return -1; - len = read(f, buf, sizeof(buf)-1); - close(f); - if (len <= 0) - return -1; - buf[len] = '\0'; - - pos = strstr(buf, "procs_running "); - if (pos == NULL) - return -1; + while (fgets(buf, sizeof(buf), f) != NULL) { + int value; - if (sscanf(pos, "procs_running %u", &running) != 1) - return -1; + if (sscanf(buf, "procs_running %u", &value) == 1) { + running = value; + break; + } + } + fclose(f); return running; } --=-9uVwBauO7C6cQUFF2372 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ --=-9uVwBauO7C6cQUFF2372 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net Linux-hotplug-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel --=-9uVwBauO7C6cQUFF2372--