From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> To: Dave Hansen <dave.hansen@intel.com> Cc: Andrew Morton <akpm@linux-foundation.org>, Konstantin Khlebnikov <koct9i@gmail.com>, Wu Fengguang <fengguang.wu@intel.com>, Arnaldo Carvalho de Melo <acme@redhat.com>, Borislav Petkov <bp@alien8.de>, "Kirill A. Shutemov" <kirill@shutemov.name>, Johannes Weiner <hannes@cmpxchg.org>, Rusty Russell <rusty@rustcorp.com.au>, David Miller <davem@davemloft.net>, Andres Freund <andres@2ndquadrant.com>, linux-kernel@vger.kernel.org, linux-mm@kvack.org, Christoph Hellwig <hch@infradead.org>, Dave Chinner <david@fromorbit.com>, Michael Kerrisk <mtk.manpages@gmail.com>, Linux API <linux-api@vger.kernel.org>, Naoya Horiguchi <nao.horiguchi@gmail.com> Subject: Re: [PATCH v3 3/3] man2/fincore.2: document general description about fincore(2) Date: Mon, 7 Jul 2014 16:59:56 -0400 [thread overview] Message-ID: <20140707205956.GB5031@nhori.bos.redhat.com> (raw) In-Reply-To: <53BAF01C.8010700@intel.com> On Mon, Jul 07, 2014 at 12:08:12PM -0700, Dave Hansen wrote: > On 07/07/2014 11:00 AM, Naoya Horiguchi wrote: > > +.SH RETURN VALUE > > +On success, > > +.BR fincore () > > +returns 0. > > +On error, \-1 is returned, and > > +.I errno > > +is set appropriately. > > Is this accurate? From reading the syscall itself, it looked like it > did this: > > > + * Return value is the number of pages whose data is stored in fc->buffer. > > + */ > > +static long do_fincore(struct fincore_control *fc, int nr_pages) > > and: > > > +SYSCALL_DEFINE6(fincore, int, fd, loff_t, start, long, nr_pages, > ... > > + while (fc.nr_pages > 0) { > > + memset(fc.buffer, 0, fc.buffer_size); > > + ret = do_fincore(&fc, min(step, fc.nr_pages)); > > + /* Reached the end of the file */ > > + if (ret == 0) > > + break; > > + if (ret < 0) > > + break; > ... > > + } > ... > > + return ret; > > +} > > Which seems that for a given loop of do_fincore(), you might end up > returning the result of that *single* iteration of do_fincore() instead > of the aggregate of the entire syscall. > > So, it can return <0 on failure, 0 on success, or also an essentially > random >0 number on success too. We don't break this while loop if do_fincore() returned a positive value unless copy_to_user() fails. And in that case ret is set to -EFAULT. So I think sys_fincore() never returns a positive value. BTW, we don't have to check "if (ret == 0)" and "if (ret < 0)" separately, I'll fix it. > Why not just use the return value for something useful instead of > hacking in the extras->nr_entries stuff? Hmm, I got the opposite complaint previously, where we shouldn't interpret the return value differently depending on the flag. And I'd like to keep the extra argument for future extensibility. For example, if we want to collect pages only with a specific set of page flags, this extra argument will be necessary. > Oh, and what if that > > > + if (extra) > > + __put_user(nr, &extra->nr_entries); > > fails? It seems like we might silently forget to tell userspace how > many entries we filled. Oh, I forget to check it. Thanks, Naoya Horiguchi
WARNING: multiple messages have this Message-ID (diff)
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> To: Dave Hansen <dave.hansen@intel.com> Cc: Andrew Morton <akpm@linux-foundation.org>, Konstantin Khlebnikov <koct9i@gmail.com>, Wu Fengguang <fengguang.wu@intel.com>, Arnaldo Carvalho de Melo <acme@redhat.com>, Borislav Petkov <bp@alien8.de>, "Kirill A. Shutemov" <kirill@shutemov.name>, Johannes Weiner <hannes@cmpxchg.org>, Rusty Russell <rusty@rustcorp.com.au>, David Miller <davem@davemloft.net>, Andres Freund <andres@2ndquadrant.com>, linux-kernel@vger.kernel.org, linux-mm@kvack.org, Christoph Hellwig <hch@infradead.org>, Dave Chinner <david@fromorbit.com>, Michael Kerrisk <mtk.manpages@gmail.com>, Linux API <linux-api@vger.kernel.org>, Naoya Horiguchi <nao.horiguchi@gmail.com> Subject: Re: [PATCH v3 3/3] man2/fincore.2: document general description about fincore(2) Date: Mon, 7 Jul 2014 16:59:56 -0400 [thread overview] Message-ID: <20140707205956.GB5031@nhori.bos.redhat.com> (raw) In-Reply-To: <53BAF01C.8010700@intel.com> On Mon, Jul 07, 2014 at 12:08:12PM -0700, Dave Hansen wrote: > On 07/07/2014 11:00 AM, Naoya Horiguchi wrote: > > +.SH RETURN VALUE > > +On success, > > +.BR fincore () > > +returns 0. > > +On error, \-1 is returned, and > > +.I errno > > +is set appropriately. > > Is this accurate? From reading the syscall itself, it looked like it > did this: > > > + * Return value is the number of pages whose data is stored in fc->buffer. > > + */ > > +static long do_fincore(struct fincore_control *fc, int nr_pages) > > and: > > > +SYSCALL_DEFINE6(fincore, int, fd, loff_t, start, long, nr_pages, > ... > > + while (fc.nr_pages > 0) { > > + memset(fc.buffer, 0, fc.buffer_size); > > + ret = do_fincore(&fc, min(step, fc.nr_pages)); > > + /* Reached the end of the file */ > > + if (ret == 0) > > + break; > > + if (ret < 0) > > + break; > ... > > + } > ... > > + return ret; > > +} > > Which seems that for a given loop of do_fincore(), you might end up > returning the result of that *single* iteration of do_fincore() instead > of the aggregate of the entire syscall. > > So, it can return <0 on failure, 0 on success, or also an essentially > random >0 number on success too. We don't break this while loop if do_fincore() returned a positive value unless copy_to_user() fails. And in that case ret is set to -EFAULT. So I think sys_fincore() never returns a positive value. BTW, we don't have to check "if (ret == 0)" and "if (ret < 0)" separately, I'll fix it. > Why not just use the return value for something useful instead of > hacking in the extras->nr_entries stuff? Hmm, I got the opposite complaint previously, where we shouldn't interpret the return value differently depending on the flag. And I'd like to keep the extra argument for future extensibility. For example, if we want to collect pages only with a specific set of page flags, this extra argument will be necessary. > Oh, and what if that > > > + if (extra) > > + __put_user(nr, &extra->nr_entries); > > fails? It seems like we might silently forget to tell userspace how > many entries we filled. Oh, I forget to check it. Thanks, Naoya Horiguchi -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2014-07-07 21:01 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-07-07 18:00 [PATCH v3 0/3] mm: introduce fincore() v3 Naoya Horiguchi 2014-07-07 18:00 ` Naoya Horiguchi 2014-07-07 18:00 ` [PATCH v3 1/3] mm: introduce fincore() Naoya Horiguchi 2014-07-07 18:00 ` Naoya Horiguchi 2014-07-07 19:01 ` Dave Hansen 2014-07-07 19:01 ` Dave Hansen 2014-07-07 20:21 ` Naoya Horiguchi 2014-07-07 20:21 ` Naoya Horiguchi 2014-07-07 20:43 ` Dave Hansen 2014-07-07 20:43 ` Dave Hansen 2014-07-07 21:48 ` Naoya Horiguchi 2014-07-07 21:48 ` Naoya Horiguchi 2014-07-07 22:44 ` Dave Hansen 2014-07-07 22:44 ` Dave Hansen 2014-07-08 15:35 ` Naoya Horiguchi 2014-07-08 15:35 ` Naoya Horiguchi 2014-07-08 19:03 ` Naoya Horiguchi 2014-07-08 19:03 ` Naoya Horiguchi 2014-07-08 19:42 ` Dave Hansen 2014-07-08 19:42 ` Dave Hansen 2014-07-08 20:41 ` Naoya Horiguchi 2014-07-08 20:41 ` Naoya Horiguchi 2014-07-08 22:32 ` Dave Hansen 2014-07-08 22:32 ` Dave Hansen 2014-07-11 16:53 ` Naoya Horiguchi 2014-07-11 16:53 ` Naoya Horiguchi 2014-07-07 18:00 ` [PATCH v3 2/3] selftests/fincore: add test code for fincore() Naoya Horiguchi 2014-07-07 18:00 ` Naoya Horiguchi 2014-07-07 18:00 ` [PATCH v3 3/3] man2/fincore.2: document general description about fincore(2) Naoya Horiguchi 2014-07-07 18:00 ` Naoya Horiguchi 2014-07-07 19:08 ` Dave Hansen 2014-07-07 19:08 ` Dave Hansen 2014-07-07 19:08 ` Dave Hansen 2014-07-07 20:59 ` Naoya Horiguchi [this message] 2014-07-07 20:59 ` Naoya Horiguchi 2014-07-07 22:34 ` Dave Hansen 2014-07-07 22:34 ` Dave Hansen 2014-07-08 15:43 ` Naoya Horiguchi 2014-07-08 15:43 ` Naoya Horiguchi 2014-07-08 12:16 ` [PATCH v3 0/3] mm: introduce fincore() v3 Christoph Hellwig 2014-07-08 12:16 ` Christoph Hellwig 2014-07-08 13:27 ` Naoya Horiguchi 2014-07-08 13:27 ` Naoya Horiguchi 2014-07-09 8:51 ` Christoph Hellwig 2014-07-09 8:51 ` Christoph Hellwig
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=20140707205956.GB5031@nhori.bos.redhat.com \ --to=n-horiguchi@ah.jp.nec.com \ --cc=acme@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=andres@2ndquadrant.com \ --cc=bp@alien8.de \ --cc=dave.hansen@intel.com \ --cc=davem@davemloft.net \ --cc=david@fromorbit.com \ --cc=fengguang.wu@intel.com \ --cc=hannes@cmpxchg.org \ --cc=hch@infradead.org \ --cc=kirill@shutemov.name \ --cc=koct9i@gmail.com \ --cc=linux-api@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mtk.manpages@gmail.com \ --cc=nao.horiguchi@gmail.com \ --cc=rusty@rustcorp.com.au \ /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: linkBe 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.