Re: [bug-gnulib] valloc()?
Derek Price wrote:
But it doesn't do so if malloc() fails on non-POSIX systems (like mingw
or so). I think one should set errno = ENOMEM if malloc() fails.
How about the attached patch?
+ errno = 0;
+ unaligned_ptr = malloc (size + pagesize - 1);
+ if (unaligned_ptr == NULL)
+{
+ /* Failed malloc on some non-posix systems (e.g. mingw) fail to set
+ errno. */
+ if (!errno) errno = ENOMEM;
+ return NULL;
+}
malloc() can (and usually does) cause system calls that can modify errno.
I.e. on an ISO C compliant implementation, errno can be anything after
malloc() returns. I.e. it is not meaningful.
Also, looking at the value of _POSIX_C_SOURCE to determine whether malloc()
will set errno or not is doomed, because even in mingw, people can
#define _POSIX_C_SOURCE 2
or similar.
So the only thing I consider possible is the appended patch.
Bruno
diff -c -3 -r1.4 pagealign_alloc.c
*** pagealign_alloc.c 3 Mar 2005 20:38:38 - 1.4
--- pagealign_alloc.c 4 Mar 2005 12:23:25 -
***
*** 149,155
size_t pagesize = getpagesize ();
void *unaligned_ptr = malloc (size + pagesize - 1);
if (unaligned_ptr == NULL)
! return NULL;
ret = (char *) unaligned_ptr
+ ((- (unsigned long) unaligned_ptr) (pagesize - 1));
new_memnode (ret, unaligned_ptr);
--- 149,160
size_t pagesize = getpagesize ();
void *unaligned_ptr = malloc (size + pagesize - 1);
if (unaligned_ptr == NULL)
! {
! /* Set errno. We don't know whether malloc already set errno: some
!implementations of malloc do, some don't. */
! errno = ENOMEM;
! return NULL;
! }
ret = (char *) unaligned_ptr
+ ((- (unsigned long) unaligned_ptr) (pagesize - 1));
new_memnode (ret, unaligned_ptr);
___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Bruno Haible wrote: | So the only thing I consider possible is the appended patch. Ok. Regards, Derek -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.0 (Cygwin) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCKIJCLD1OTBfyMaQRAgapAJ9KNYCgAYxDb/aTYvKBIVAr/fQ7OwCgybG6 ewcJs/AJoM5j+hW74C3ETFo= =vQ91 -END PGP SIGNATURE- ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Derek Price wrote: | Re my earlier questions about efficiency, here's the patch using | the new pagealign_alloc module which is currently passing `make | remotecheck' here. It also contains a few modification to the new | GNULIB pagealign_alloc module because I haven't finalized them and | submitted them to bug-gnulib yet, but I should shortly. | | Comments or benchmarks are appreciated. Okay, I've committed this with the benchmark request in NEWS. It can always be backed out if it proves to cause trouble. Regards, Derek -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.0 (Cygwin) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCKLE9LD1OTBfyMaQRAqFXAKC5J5HFfHAufl+oQ8XiAtZmljQ3PwCgzh0y 0qA5yu6viyyxvFHqTQ9c+n4= =PXb+ -END PGP SIGNATURE- ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
Derek Price wrote:
First, I found man pages stating that valloc() was obsoleted in favor of
posix_memalign(). Does anyone have a feel for how portable
posix_memalign() is
It appears to be implemented only by glibc.
Second, if posix_memalign() is not portable, is there any interest in a
GNULIB module either to replace it or valloc()? I've attached the
valloc.c that CVS is currently using.
The spec says that the returned memory must be freeable through free().
I don't see how you can write a portable replacement for posix_memalign()
without digging in undocumented details of the malloc() implementation.
void *
valloc (bytes)
size_t bytes;
{
long pagesize;
void *ret;
pagesize = getpagesize ();
ret = malloc (bytes + pagesize - 1);
if (ret)
ret = (long) (ret + pagesize - 1) ~ (pagesize - 1);
return ret;
}
This is the best you can do in a portable way, but
1. the return value cannot be passed to free(),
2. it wastes 1/2 page of memory on average.
Therefore I'd suggest a new interface:
void* pagealign_alloc(size_t);
void pagealign_free(void*);
and do the implementation as follows:
- If mmap() is available, use mmap and some bookkeeping for pagealign_alloc,
and munmap() for pagealign_free,
- Otherwise, if posix_memalign() is available, use it and free(),
- Otherwise, use something similar to the valloc() above.
Is this all worth it? For what purpose do you need the memory to be
page-aligned?
Bruno
___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
Derek Price wrote: Why mmap? mmap allocates the number of pages you want, without the 1/2 page lossage on average that malloc() has. What sort of object would you suggest I map the allocation to? My Linux man page says that the MAP_ANONYMOUS flag (which requires no object to map) has been supported since the 2.4 kernel, but I can find no reference to MAP_ANONYMOUS in the POSIX spec. Different systems want it differently. I use an autoconf test to determine which way to use mmap(): 1) int flags = MAP_ANON | MAP_PRIVATE; int fd = -1; 2) int flags = MAP_ANONYMOUS | MAP_PRIVATE; int fd = -1; 3) #ifndef MAP_FILE #define MAP_FILE 0 #endif int flags = MAP_FILE | MAP_PRIVATE; int fd = open(/dev/zero,O_RDONLY,0666); if (fd0) exit(1); These three together cover all systems that have mmap(). (Most of this is hardly necessary for CVS since it is already caching unused buffer datas rather than freeing them, and reusing them as they are needed. This is actually a bad idea, because 1) It steals memory from the system. If a program doesn't use memory but still holds it, it forces other programs to do more page faults. 2) When memory is tight, it forces the system to write garbage data to swap space. 3) It slows down 'cvs' itself. Because when the page gets swapped out to disk, and the the program notices that it needs it, the OS must re-fetch the page from the swap. This usually takes ca. 10 ms. Whereas when a program munmap()s the pages that it doesn't need any more, 1) Other programs can use the free mempry. 3) When the program needs more memory, the kernel can often provide a blank page immediately, without having to wait for a swapped out page to come in. Bruno ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
Derek Price wrote:
Why do you prefer mmap to posix_memalign?
Yes, if posix_memalign exists, it can probably be used instead of mmap.
But since the only known platform having it is glibc, and on glibc it
wastes even more memory than your simple valloc(), I would prefer raw mmap().
This simple program (on x86)
#include stdlib.h
int main () {
void* p;
posix_memalign(p, 4096, 2*4096);
posix_memalign(p, 4096, 10*4096);
posix_memalign(p, 4096, 100*4096);
return 0;
}
produces this strace:
brk(0) = 0x8049580
brk(0x804a580) = 0x804a580
brk(0) = 0x804a580
brk(0x804b000) = 0x804b000
brk(0) = 0x804b000
brk(0x804d000) = 0x804d000
brk(0) = 0x804d000
brk(0x8058000) = 0x8058000
mmap2(NULL, 417792, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x40156000
I.e. when asked for 2 pages, it allocates 2 pages.
when asked for 10 pages, it allocates 11 pages.
when asked for 100 pages, it allocates 102 pages.
it strikes me that my
bookkeeping for mmap could slow things down if many blocks were
allocated (I'm storing the ptr-size map in a simple linked list, but
even something more efficient would eventually slow).
There's a GPLed implementation of red-black trees in the Linux kernel.
Red-black trees, like AVL trees, have an O(log N) worst case runtime for
lookup, insertion, removal. I don't consider this slow.
Bruno
___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
Derek Price wrote:
Okay, I've implemented this as you suggested, Bruno. Installed in CVS,
it passes tests in all four modes (MMAP, MMAP/NO-MAP_ANON,
POSIX_MEMALIGN, OTHER). I've attached the patch, but I still have a few
questions.
Thanks. I've installed this into gnulib, with the following minor
modifications:
- Indentation according to GNU standards.
- pagealign_alloc.h:
- Add comments.
- Include stddef.h, for size_t.
- pagealign_alloc.c:
- Remove the mention of public domain since it contradicts the GPL.
- Include pagealign_alloc.h first, to verify that it is self-consistent.
- Use the 'exit' module for EXIT_FAILURE.
- Don't use EINVAL in an error message that is already verbose enough.
- Drop the trailing dot in error messages that use an errno, since
they are displayed with a colon and the errno explanation following it.
- Fixed portability problem of void* computations like 'orig + pagesize -
1'.
- Use a typedef to avoid many #ifs.
- Rename input parameter 'out' to 'aligned_ptr'. (An input parameter called
'out' is somewhat paradox.)
- Don't use 'new' as variable name, since we might want to compile the
code with a C++ compiler some day.
- Removed unnecessary fields from 'memtable' and renamed it to
'memnode_table'.
- In get_memnode, use abort() instead of exit() to signal a bug in the
calling code.
- Make pagealign_alloc work also if a 'void *' does not fit in a 'long'.
- Use #if HAVE_POSIX_MEMALIGN consistently, not a mix of #if here and
#ifdef there.
- mmap.m4: Renamed to mmap-anon.m4 since it cares only about anonymous
mappings, not about file or shared mappings. ('grep' has a different
test for mmap, and 'clisp' yet another one.)
Write config.h, not config.h.
Do you still see some nits that could be improved?
Bruno
=== modules/pagealign_alloc
Description:
Memory allocation aligned on page boundaries.
Files:
lib/pagealign_alloc.h
lib/pagealign_alloc.c
m4/mmap-anon.m4
m4/pagealign_alloc.m4
Depends-on:
error
exit
getpagesize
xalloc
configure.ac:
gl_PAGEALIGN_ALLOC
Makefile.am:
Include:
#include pagealign_alloc.h
License:
GPL
Maintainer:
[EMAIL PROTECTED]
= lib/pagealign_alloc.h
/* Memory allocation aligned to system page boundaries.
Copyright (C) 2005 Free Software Foundation, Inc.
This program is free software; you can redistribute it and/or modify it
under the terms of the GNU General Public License as published
by the Free Software Foundation; either version 2, or (at your option)
any later version.
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
General Public License for more details.
You should have received a copy of the GNU General Public
License along with this program; if not, write to the Free Software
Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
USA. */
#ifndef _PAGEALIGN_ALLOC_H
# define _PAGEALIGN_ALLOC_H
# include stddef.h
/* Allocate a block of memory of SIZE bytes, aligned on a system page
boundary.
If SIZE is not a multiple of the system page size, it will be rounded up
to the next multiple.
Return a pointer to the start of the memory block, or NULL if the allocation
failed. */
extern void *pagealign_alloc (size_t size);
/* Free a memory block.
PTR must be a pointer returned by pagealign_alloc. */
extern void pagealign_free (void *ptr);
#endif /* _PAGEALIGN_ALLOC_H */
= lib/pagealign_alloc.c
/* Memory allocation aligned to system page boundaries.
Copyright (C) 2005 Free Software Foundation, Inc.
This program is free software; you can redistribute it and/or modify it
under the terms of the GNU General Public License as published
by the Free Software Foundation; either version 2, or (at your option)
any later version.
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
General Public License for more details.
You should have received a copy of the GNU General Public
License along with this program; if not, write to the Free Software
Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
USA. */
/* Written by Derek R. Price [EMAIL PROTECTED]. */
#ifdef HAVE_CONFIG_H
# include config.h
#endif
#include pagealign_alloc.h
#include errno.h
#include stdlib.h
#if HAVE_FCNTL_H
# include fcntl.h
#endif
#if HAVE_UNISTD_H
# include unistd.h
#endif
#if HAVE_MMAP
# include sys/mman.h
#endif
#include error.h
#include exit.h
#include getpagesize.h
#include xalloc.h
#if
Re: [bug-gnulib] valloc()?
Derek Price wrote:
- Rename macro gl_FUNC_MMAP to gl_FUNC_MMAP_ANON to sync
with new m4 file name.
Applied, thanks.
- Remove unnecessary fd set.
You can even optimize away the 'fd' variable entirely in this case and
make it a constant.
- Comment in header says pagealign_alloc() may return NULL. It may not -
it will exit via error() instead.
Ah, good point. Coming to think of it:
- The details of the error messages (mmap to /dev/zero failed,
posix_memalign failed) both boil down to the same, namely
memory exhausted. For an end user, memory exhausted is more
understandable, so let's use this.
- Some programs want to do custom actions when an out-of-memory
situation occurs. That's why we have xalloc_die(). So xalloc_die() should
be called here instead of doing error(EXIT_FAILURE,0,memory exhausted).
- Some programs may want to have a pagealign_alloc() that returns NULL
upon failure. Should pagealign_alloc() therefore return NULL or call
xalloc_die()?
- The answer is clear: We usually use an 'x' in the function name ('x'
means 'checking') when xalloc_die() may be called. Therefore if we
provide a function pagealign_alloc() which returns NULL upon failure
and a function pagealign_xalloc() which calls xalloc_die() in this
case, both needs are covered.
So I committed the appended patch.
And one question:
Why do you cast to char * here, when ret is actually a void *? Is this
necessary?
void *unaligned_ptr = xmalloc (size + pagesize - 1);
ret = (char *) unaligned_ptr
+ ((- (unsigned long) unaligned_ptr) (pagesize - 1));
This is necessary because 'void *' + 'integer' is not defined in ANSI C nor
ISO C99.
Only GCC interprets it the same way as 'char *' + 'integer'; all other
compilers give an error.
Bruno
diff -c -3 -r1.1 pagealign_alloc.h
*** pagealign_alloc.h 3 Mar 2005 14:07:04 - 1.1
--- pagealign_alloc.h 3 Mar 2005 16:19:47 -
***
*** 30,37
failed. */
extern void *pagealign_alloc (size_t size);
/* Free a memory block.
!PTR must be a pointer returned by pagealign_alloc. */
extern void pagealign_free (void *ptr);
#endif /* _PAGEALIGN_ALLOC_H */
--- 30,42
failed. */
extern void *pagealign_alloc (size_t size);
+ /* Like pagealign_alloc, except it exits the program if the allocation
+fails. */
+ extern void *pagealign_xalloc (size_t size);
+
/* Free a memory block.
!PTR must be a non-NULL pointer returned by pagealign_alloc or
!pagealign_xalloc. */
extern void pagealign_free (void *ptr);
#endif /* _PAGEALIGN_ALLOC_H */
diff -c -3 -r1.1 pagealign_alloc.c
*** pagealign_alloc.c 3 Mar 2005 14:07:04 - 1.1
--- pagealign_alloc.c 3 Mar 2005 16:19:47 -
***
*** 117,129
void *ret;
#if HAVE_MMAP
int flags;
- static int fd = -1; /* Only open /dev/zero once in order to avoid limiting
- the amount of memory we may allocate based on the
- number of open file descriptors. */
# ifdef HAVE_MAP_ANONYMOUS
flags = MAP_ANONYMOUS | MAP_PRIVATE;
- fd = -1;
# else /* !HAVE_MAP_ANONYMOUS */
flags = MAP_FILE | MAP_PRIVATE;
if (fd == -1)
{
--- 117,129
void *ret;
#if HAVE_MMAP
int flags;
# ifdef HAVE_MAP_ANONYMOUS
+ const int fd = -1;
flags = MAP_ANONYMOUS | MAP_PRIVATE;
# else /* !HAVE_MAP_ANONYMOUS */
+ static int fd = -1; /* Only open /dev/zero once in order to avoid limiting
+ the amount of memory we may allocate based on the
+ number of open file descriptors. */
flags = MAP_FILE | MAP_PRIVATE;
if (fd == -1)
{
***
*** 134,148
# endif /* HAVE_MAP_ANONYMOUS */
ret = mmap (NULL, size, PROT_READ | PROT_WRITE, flags, fd, 0);
if (!ret)
! error (EXIT_FAILURE, errno, mmap to /dev/zero failed);
new_memnode (ret, size);
#elif HAVE_POSIX_MEMALIGN
int status = posix_memalign (ret, getpagesize (), size);
if (status)
! error (EXIT_FAILURE, status, posix_memalign failed);
#else /* !HAVE_MMAP !HAVE_POSIX_MEMALIGN */
size_t pagesize = getpagesize ();
! void *unaligned_ptr = xmalloc (size + pagesize - 1);
ret = (char *) unaligned_ptr
+ ((- (unsigned long) unaligned_ptr) (pagesize - 1));
new_memnode (ret, unaligned_ptr);
--- 134,150
# endif /* HAVE_MAP_ANONYMOUS */
ret = mmap (NULL, size, PROT_READ | PROT_WRITE, flags, fd, 0);
if (!ret)
! return NULL;
new_memnode (ret, size);
#elif HAVE_POSIX_MEMALIGN
int status = posix_memalign (ret, getpagesize (), size);
if (status)
! return NULL;
#else /* !HAVE_MMAP !HAVE_POSIX_MEMALIGN */
size_t pagesize = getpagesize ();
! void *unaligned_ptr = malloc (size + pagesize - 1);
! if (unaligned_ptr == NULL)
! return NULL;
ret = (char *) unaligned_ptr
+ ((-
Re: [bug-gnulib] valloc()?
Do you still see some nits that could be improved?
Oops, there was one more nit: The error messages were not internationalized.
I did this:
diff -c -3 -r1.2 pagealign_alloc.c
*** lib/pagealign_alloc.c 3 Mar 2005 16:21:00 - 1.2
--- lib/pagealign_alloc.c 3 Mar 2005 16:36:58 -
***
*** 44,49
--- 44,52
#include exit.h
#include getpagesize.h
#include xalloc.h
+ #include gettext.h
+
+ #define _(str) gettext (str)
#if HAVE_MMAP || ! HAVE_POSIX_MEMALIGN
***
*** 129,135
{
fd = open (/dev/zero, O_RDONLY, 0666);
if (fd 0)
! error (EXIT_FAILURE, errno, Failed to open /dev/zero for read);
}
# endif /* HAVE_MAP_ANONYMOUS */
ret = mmap (NULL, size, PROT_READ | PROT_WRITE, flags, fd, 0);
--- 132,138
{
fd = open (/dev/zero, O_RDONLY, 0666);
if (fd 0)
! error (EXIT_FAILURE, errno, _(Failed to open /dev/zero for read));
}
# endif /* HAVE_MAP_ANONYMOUS */
ret = mmap (NULL, size, PROT_READ | PROT_WRITE, flags, fd, 0);
diff -c -3 -r1.1 pagealign_alloc
*** modules/pagealign_alloc 3 Mar 2005 14:07:04 - 1.1
--- modules/pagealign_alloc 3 Mar 2005 16:36:58 -
***
*** 11,16
--- 11,17
error
exit
getpagesize
+ gettext
xalloc
configure.ac:
___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Bruno Haible wrote:
| Do you still see some nits that could be improved?
|
|
| Oops, there was one more nit: The error messages were not
| internationalized. I did this:
[snip]
| *** *** 129,135 { fd = open (/dev/zero,
| O_RDONLY, 0666); if (fd 0) ! error (EXIT_FAILURE, errno, Failed
| to open /dev/zero for read); } # endif /* HAVE_MAP_ANONYMOUS */
| ret = mmap (NULL, size, PROT_READ | PROT_WRITE, flags, fd, 0); ---
| 132,138 { fd = open (/dev/zero, O_RDONLY, 0666); if (fd 0)
| ! error (EXIT_FAILURE, errno, _(Failed to open /dev/zero for
| read)); } # endif /* HAVE_MAP_ANONYMOUS */ ret = mmap (NULL, size,
| PROT_READ | PROT_WRITE, flags, fd, 0);
Should this error still be exiting (EXIT_FAILURE) when this function
is defined to return NULL on failure?
Regards,
Derek
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.0 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFCJ0j/LD1OTBfyMaQRAtohAJ9eNQ/a2dO1wXrfdw4WKfztB5UTzwCgjPdP
F3Y6HTeZBnfMFD6SZ5AdOqQ=
=zhqn
-END PGP SIGNATURE-
___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Re my earlier questions about efficiency, here's the patch using the new pagealign_alloc module which is currently passing `make remotecheck' here. It also contains a few modification to the new GNULIB pagealign_alloc module because I haven't finalized them and submitted them to bug-gnulib yet, but I should shortly. Comments or benchmarks are appreciated. Regards, Derek -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.0 (Cygwin) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCJ0qWLD1OTBfyMaQRAg15AJ0X15wNNTvYvL7i+yVr9lkycBGlkgCg0wqw bbMrUmS9W1T3vBWiXIJDLA0= =hynq -END PGP SIGNATURE- ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Bruno Haible wrote: | Derek Price wrote: | | Should this error still be exiting (EXIT_FAILURE) when this | function is defined to return NULL on failure? | | | In this case the error is not memory exhausted, it is cannot | open /dev/zero. This can happen for example if someone is working | in a chroot environment that is incorrectly set up. I think a clear | error message is good in this case. But I agree it's hard to say in | advance which behaviour is better. It could print the error message with error (0, errno, ...) and then return NULL. The caller could then decide if that error should be fatal or not, as I presume they might wish to if they are calling pagealign_alloc() as opposed to pagealign_xalloc(). Regards, Derek -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.0 (Cygwin) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCJ188LD1OTBfyMaQRAisLAKCjzfZm7V5uPkRIXQ3ILJSLhHIHRwCgkNSz qL4MjglKCWi/7OEocliSR28= =ubwl -END PGP SIGNATURE- ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
Derek Price wrote: It could print the error message with error (0, errno, ...) and then return NULL. The caller could then decide if that error should be fatal or not, as I presume they might wish to if they are calling pagealign_alloc() as opposed to pagealign_xalloc(). Maybe... but it makes things more complicated for the caller. And if this error occurs, it's a problem in the environment, not in the program's data; and since it occurs upon the first call to pagealign_alloc, you can assume that the program does not have sensitive data to save. Bruno ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
Derek Price wrote: I've attached a patch to fix a few more nits Committed, with small wording tweaks in the comments. - I noticed that I assumed mmap() returns NULL on error when it actually returns MAP_FAILED according to POSIX. Ouch, this was a major bug. Glad that you found it. - Note in the header that pagealign_alloc sets errno on failure. But it doesn't do so if malloc() fails on non-POSIX systems (like mingw or so). I think one should set errno = ENOMEM if malloc() fails. I had two further thoughts on the naming of mmap-anon.m4. First, in my original decision to name it mmap.m4, I did take into consideration that there might be other mmap.m4 implementations but, ideally, I would hope that their requirements could be merged into this mmap(-anon).m4, at the least for simplicity's sake. I don't have this hope: The requirements of different programs regarding mmap are so different that, if all tests were merged into a common mmap.m4 file, some people would say this test is insane - it disables mmap on SVR4 [or Linux 1.2 or HP-UX or ...] although it is perfectly sane. SVR4 had problems with PRIVATE READ-WRITE mappings of files. Linux 1.2 didn't support MAP_SHARED on files. HP-UX doesn't support mapping files at fixed addresses in most cases. Second, technically what we are currently calling gl_FUNC_MMAP_ANON calls AC_FUNC_MMAP, so it also verifies that private, fixed maps to files work as well, not just anonymous maps. Yes, this is one of the problems with AC_FUNC_MMAP. But fortunately we can ignore it because nowadays most systems have a working mmap(), therefore not many people will complain it disables mmap() on my system. Bruno ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Bruno Haible wrote: | Derek Price wrote: | | It could print the error message with error (0, errno, ...) and | then return NULL. The caller could then decide if that error | should be fatal or not, as I presume they might wish to if they | are calling pagealign_alloc() as opposed to pagealign_xalloc(). | | | Maybe... but it makes things more complicated for the caller. And | if this error occurs, it's a problem in the environment, not in the | program's data; and since it occurs upon the first call to | pagealign_alloc, you can assume that the program does not have | sensitive data to save. Good point. I'll concede that. Regards, Derek -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.0 (Cygwin) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCJ3lwLD1OTBfyMaQRAks/AJwMOHm3oSDKrg/lvQ13Y6Urwu4C+QCg6No/ a6xUjZzVmVfqLKmIGam5Qso= =MgCn -END PGP SIGNATURE- ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Bruno Haible wrote: | Is this all worth it? For what purpose do you need the memory to be | page-aligned? That's a good question, as the original isn't my code. I was just assuming that whoever wrote it originally knew what they were doing. There is only one place that CVS does this, and it is where it is allocating new, empty buffer datas. It later uses theres primarily in calls to read() from various sources, from network pipes, exec pipes, to files. Looking at it, it mitigates the average 1/2 page lossage by allocating 16 * 4096 byte buffers at once, but this also means that it is not making any attempt to force later buffer datas to be page aligned - if this is compiled on a system with a page size greater than 4096, some of the buffer datas will not be page aligned. Is there any good reason for doing this? The buffers are saved and reused, and their ordering is not preserved, so I wouldn't expect a great performance advantage from doing this. Regards, Derek -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.0 (Cygwin) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCJeIYLD1OTBfyMaQRAkYWAKC4ltX+X3gGVmHV2bidNWC1Z2Z2RQCgzern 32GIGKRl3OoOKZtFQbftwqI= =g/7U -END PGP SIGNATURE- ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
Re: [bug-gnulib] valloc()?
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 I asked Jim Kingdon, who I thought I might be able to blame for the original code, why valloc() was chosen. He replied: | Well, if the buffer.c buffers are split across pages, you might | have two page faults instead of one, the kernel would have to | maintain two page table entries instead of one, etc. So in that | sense, page-aligned is the right thing, but not at all a big deal | since we are allocating 4096*16 bytes at a time. | | Some networking kernels can probably avoid a copy of the data if | you pass a page-aligned buffer to system calls like | recv/send/read/write, which might be a bigger performance win. | | Now, many versions of malloc will page-align an allocation that big | without us having to ask for it. | | The verdict? valloc (or its relatives like posix_memalign) isn't | worth the portability hassles. Although I suppose if you have | doubts you could try to come up with some suitable benchmark (you'd | have to figure out what would hit buffer.c hard enough, and find a | malloc which would misalign pages for you). | | (None of this is based on my memory of why that code got into CVS - | presumably because Ian wrote it that way and no one thought to | advocate changing it). Based on this, I might choose to implement your suggestion, Bruno (and change the default buffer data size in CVS to the system page size): | This is the best you can do in a portable way, but 1. the return | value cannot be passed to free(), 2. it wastes 1/2 page of memory | on average. | | Therefore I'd suggest a new interface: void* | pagealign_alloc(size_t); void pagealign_free(void*); | | and do the implementation as follows: - If mmap() is available, use | mmap and some bookkeeping for pagealign_alloc, and munmap() for | pagealign_free, - Otherwise, if posix_memalign() is available, use | it and free(), - Otherwise, use something similar to the valloc() | above. But I have a few questions. Why mmap? What sort of object would you suggest I map the allocation to? My Linux man page says that the MAP_ANONYMOUS flag (which requires no object to map) has been supported since the 2.4 kernel, but I can find no reference to MAP_ANONYMOUS in the POSIX spec. (Most of this is hardly necessary for CVS since it is already caching unused buffer datas rather than freeing them, and reusing them as they are needed. All we really need is the valloc() module, though it might be nice to default to posix_memalign() (or mmap()?) instead, when one can be found, simply to avoid the wasted space before the page.) Regards, Derek Derek Price wrote: | Bruno Haible wrote: | | | Is this all worth it? For what purpose do you need the memory to | be | page-aligned? | | | That's a good question, as the original isn't my code. I was just | assuming that whoever wrote it originally knew what they were | doing. | | There is only one place that CVS does this, and it is where it is | allocating new, empty buffer datas. It later uses theres primarily | in calls to read() from various sources, from network pipes, exec | pipes, to files. | | Looking at it, it mitigates the average 1/2 page lossage by | allocating 16 * 4096 byte buffers at once, but this also means that | it is not making any attempt to force later buffer datas to be page | aligned - if this is compiled on a system with a page size greater | than 4096, some of the buffer datas will not be page aligned. Is | there any good reason for doing this? The buffers are saved and | reused, and their ordering is not preserved, so I wouldn't expect a | great performance advantage from doing this. | | Regards, | | Derek -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.0 (Cygwin) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCJe63LD1OTBfyMaQRAr9GAKCSMQzfkcaOs6pwagc9e1uqk4jwlACgz7ym fL9XWHl5cHBVkxfHhicBFT4= =W5eY -END PGP SIGNATURE- ___ Bug-cvs mailing list Bug-cvs@gnu.org http://lists.gnu.org/mailman/listinfo/bug-cvs
