On Fri, Oct 24, 2008 at 03:28:49PM +0100, Andy Whitcroft wrote:
> On Fri, Oct 24, 2008 at 02:01:58PM +0100, Mel Gorman wrote:
> > On Thu, Oct 23, 2008 at 03:54:59PM +0100, Andy Whitcroft wrote:
> > > gethugepagesizes has two modes: a counting mode which tells us exactly
> > > how many entries we would need; and a lookup mode which fills in a
> > > supplied buffer.
> > >
> > > The counting mode is defined as triggering when we pass in both a zero
> > > element count and a NULL buffer pointer. Currently we enter this mode
> > > when
> > > the element count is zero which causes it to trigger at the edge case
> > > where
> > > the caller (pointlessly) passes in a buffer with an element count of zero.
> > > Fix behaviour to match the documentation.
> > >
> > > Signed-off-by: Andy Whitcroft <[EMAIL PROTECTED]>
> > > ---
> > > hugeutils.c | 19 ++++++++++---------
> > > 1 files changed, 10 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/hugeutils.c b/hugeutils.c
> > > index ae60f39..eb0b7b2 100644
> > > --- a/hugeutils.c
> > > +++ b/hugeutils.c
> > > @@ -581,11 +581,11 @@ int gethugepagesizes(long pagesizes[], int n_elem)
> > > if (default_size < 0)
> > > return 0;
> > >
> > > - if (n_elem && pagesizes)
> > > + if (pagesizes && (nr_sizes == n_elem))
> > > + return nr_sizes;
> >
> > Ok, this is a little confusing because nr_sizes == 0 - can't we just go
> > pagesizes && n_elem == 0 ?
>
> well its consistent with the other version of this check in this
> function which must use nr_sizes.
>
Ok, fair enough. When applied it was obvious. It was looking at it in
patch form that I was scratching my head.
> > Otherwise, the behaviour appears to match the documentation. If you
> > supply a buffer with zero elements, then 0 pagesizes are put in the
> > buffer.
> >
> > > + if (pagesizes)
> > > pagesizes[nr_sizes] = default_size;
> >
> > I'm not seeing why you have to check pagesizes here. Earlier in the
> > function, we already check it for EINVAL and the check above would exit
> > if n_elem was 0 so we know the array is valid at this point, right?
>
> If we are in counting mode, pagesizes is null and we cannot write to it,
> but we must continue to accumulate nr_sizes.
>
D'OH, I wasn't taking the counting use of the function into account.
*sleps forehead*.
> > > nr_sizes++;
> > > - if (n_elem && (nr_sizes == n_elem))
> > > - return nr_sizes;
> > >
> > > /*
> > > * Scan sysfs to look for other sizes.
> > > @@ -599,8 +599,7 @@ int gethugepagesizes(long pagesizes[], int n_elem)
> > > } else
> > > return -1;
> > > }
> > > - while ((ent = readdir(sysfs)) &&
> > > - ((n_elem == 0) || (nr_sizes < n_elem))) {
> > > + while ((ent = readdir(sysfs))) {
> > > long size;
> > >
> > > if (strncmp(ent->d_name, "hugepages-", 10))
> > > @@ -613,7 +612,9 @@ int gethugepagesizes(long pagesizes[], int n_elem)
> > >
> > > if (size < 0 || size == default_size)
> > > continue;
> > > - if (n_elem && pagesizes)
> > > + if (pagesizes && (nr_sizes == n_elem))
> > > + return nr_sizes;
> > > + if (pagesizes)
> > > pagesizes[nr_sizes] = size;
> >
> > Can we really reach here with pagesizes == NULL?
>
> Yes, when we call this n_elem 0, pagesizes = NULL we are in counting
> mode and iterate over all entries just counting nr_sizes.
>
> > > nr_sizes++;
> > > }
> > > @@ -636,10 +637,10 @@ int getpagesizes(long pagesizes[], int n_elem)
> > > ret = gethugepagesizes(pagesizes, n_elem);
> > > } else {
> > > /* Install the base page size. */
> > > - if (n_elem && pagesizes)
> > > + if (pagesizes && n_elem == 0)
> > > + return 0;
> >
> > Here the test is easier to follow.
>
> And I am trying to be consistant with here for that clarity.
>
> > > + if (pagesizes)
> > > pagesizes[0] = sysconf(_SC_PAGESIZE);
> > > - if (n_elem == 1)
> > > - return 1;
> > >
> > > ret = gethugepagesizes(pagesizes + 1, n_elem - 1);
> > > }
>
Acked-by: Mel Gorman <[EMAIL PROTECTED]>
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Libhugetlbfs-devel mailing list
Libhugetlbfs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
