From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18454 invoked by alias); 15 Mar 2007 08:37:52 -0000 Received: (qmail 18437 invoked by uid 22791); 15 Mar 2007 08:37:51 -0000 X-Spam-Check-By: sourceware.org Received: from sunsite.ms.mff.cuni.cz (HELO sunsite.mff.cuni.cz) (195.113.15.26) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 15 Mar 2007 08:37:44 +0000 Received: from sunsite.mff.cuni.cz (localhost.localdomain [127.0.0.1]) by sunsite.mff.cuni.cz (8.13.8/8.13.8) with ESMTP id l2F8fUWI013598; Thu, 15 Mar 2007 09:41:30 +0100 Received: (from jj@localhost) by sunsite.mff.cuni.cz (8.13.8/8.13.8/Submit) id l2F8fUHS013593; Thu, 15 Mar 2007 09:41:30 +0100 Date: Thu, 15 Mar 2007 08:37:00 -0000 From: Jakub Jelinek To: Ulrich Drepper Cc: Glibc hackers Subject: Re: [PATCH] Better fix for ifaddrs Message-ID: <20070315084129.GL1826@sunsite.mff.cuni.cz> Reply-To: Jakub Jelinek References: <20070314182000.GK1826@sunsite.mff.cuni.cz> <45F8971B.9080708@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <45F8971B.9080708@redhat.com> User-Agent: Mutt/1.4.2.2i Mailing-List: contact libc-hacker-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-hacker-owner@sourceware.org X-SW-Source: 2007-03/txt/msg00020.txt.bz2 On Wed, Mar 14, 2007 at 05:45:15PM -0700, Ulrich Drepper wrote: > Jakub Jelinek wrote: > > if you think I should use > > __libc_use_alloca even for this, please let me know. > > I fear for archs like ia64 and ppc64 where the page size can widely vary > we need this. But I think that this code should be optional, i.e., only > for those archs. No need to slow down the rest of the world. Ok, how about this then? On x86_64/i386 where PAGE_SIZE is defined, constant and <= PTHREAD_STACK_MIN/4 the malloc+free are completely optimized out by the compiler, the only bad thing there is that it will still use char *buf = alloca (4096); rather than char buf[4096]; which is a few instructions more expensive. But even that is fixable in the compiler (if alloca with a constant argument is always reachable from the entry BB, then the compiler can just create a char array with that size instead of alloca). Working around this in glibc would be ugly, hardcoding what __libc_use_alloca does internally e.g. in preprocessor directive would be bad. 2007-03-15 Jakub Jelinek * sysdeps/unix/sysv/linux/ifaddrs.c (__netlink_request): Never reallocate the buffer, instead fail for MSG_TRUNC or for EBUSY NLMSG_ERR. Instead use a page sized buffer. * sysdeps/unix/sysv/linux/check_pf.c (make_request): Use page sized buffer. --- libc/sysdeps/unix/sysv/linux/check_pf.c.jj 2007-01-03 11:04:37.000000000 +0100 +++ libc/sysdeps/unix/sysv/linux/check_pf.c 2007-03-15 09:20:11.000000000 +0100 @@ -1,5 +1,5 @@ /* Determine protocol families for which interfaces exist. Linux version. - Copyright (C) 2003, 2006 Free Software Foundation, Inc. + Copyright (C) 2003, 2006, 2007 Free Software Foundation, Inc. This file is part of the GNU C Library. The GNU C Library is free software; you can redistribute it and/or @@ -71,17 +71,38 @@ make_request (int fd, pid_t pid, bool *s memset (&nladdr, '\0', sizeof (nladdr)); nladdr.nl_family = AF_NETLINK; +#ifdef PAGE_SIZE + /* Help the compiler optimize out the malloc call if PAGE_SIZE + is constant and smaller or equal to PTHREAD_STACK_MIN/4. */ + const size_t buf_size = PAGE_SIZE; +#else + const size_t buf_size = __getpagesize (); +#endif + bool use_malloc = false; + char *buf; + + if (__libc_use_alloca (buf_size)) + buf = alloca (buf_size); + else + { + buf = malloc (buf_size); + if (buf != NULL) + use_malloc = true; + else + goto out_fail; + } + + struct iovec iov = { buf, buf_size }; + if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0, (struct sockaddr *) &nladdr, sizeof (nladdr))) < 0) - return -1; + goto out_fail; *seen_ipv4 = false; *seen_ipv6 = false; bool done = false; - char buf[4096]; - struct iovec iov = { buf, sizeof (buf) }; struct in6ailist { struct in6addrinfo info; @@ -101,10 +122,10 @@ make_request (int fd, pid_t pid, bool *s ssize_t read_len = TEMP_FAILURE_RETRY (__recvmsg (fd, &msg, 0)); if (read_len < 0) - return -1; + goto out_fail; if (msg.msg_flags & MSG_TRUNC) - return -1; + goto out_fail; struct nlmsghdr *nlmh; for (nlmh = (struct nlmsghdr *) buf; @@ -186,7 +207,7 @@ make_request (int fd, pid_t pid, bool *s { *in6ai = malloc (in6ailistlen * sizeof (**in6ai)); if (*in6ai == NULL) - return -1; + goto out_fail; *in6ailen = in6ailistlen; @@ -198,6 +219,13 @@ make_request (int fd, pid_t pid, bool *s while (in6ailist != NULL); } + if (use_malloc) + free (buf); + return 0; + +out_fail: + if (use_malloc) + free (buf); return 0; } --- libc/sysdeps/unix/sysv/linux/ifaddrs.c.jj 2007-03-13 17:42:17.000000000 +0100 +++ libc/sysdeps/unix/sysv/linux/ifaddrs.c 2007-03-15 09:18:43.000000000 +0100 @@ -122,37 +122,36 @@ int __netlink_request (struct netlink_handle *h, int type) { struct netlink_res *nlm_next; - struct netlink_res **new_nlm_list; - static volatile size_t buf_size = 4096; - char *buf; struct sockaddr_nl nladdr; struct nlmsghdr *nlmh; ssize_t read_len; bool done = false; - bool use_malloc = false; - if (__netlink_sendreq (h, type) < 0) - return -1; +#ifdef PAGE_SIZE + /* Help the compiler optimize out the malloc call if PAGE_SIZE + is constant and smaller or equal to PTHREAD_STACK_MIN/4. */ + const size_t buf_size = PAGE_SIZE; +#else + const size_t buf_size = __getpagesize (); +#endif + bool use_malloc = false; + char *buf; - size_t this_buf_size = buf_size; - size_t orig_this_buf_size = this_buf_size; - if (__libc_use_alloca (this_buf_size)) - buf = alloca (this_buf_size); + if (__libc_use_alloca (buf_size)) + buf = alloca (buf_size); else { - buf = malloc (this_buf_size); + buf = malloc (buf_size); if (buf != NULL) use_malloc = true; else goto out_fail; } - struct iovec iov = { buf, this_buf_size }; + struct iovec iov = { buf, buf_size }; - if (h->nlm_list != NULL) - new_nlm_list = &h->end_ptr->next; - else - new_nlm_list = &h->nlm_list; + if (__netlink_sendreq (h, type) < 0) + goto out_fail; while (! done) { @@ -172,48 +171,7 @@ __netlink_request (struct netlink_handle continue; if (__builtin_expect (msg.msg_flags & MSG_TRUNC, 0)) - { - if (this_buf_size >= SIZE_MAX / 2) - goto out_fail; - - nlm_next = *new_nlm_list; - while (nlm_next != NULL) - { - struct netlink_res *tmpptr; - - tmpptr = nlm_next->next; - free (nlm_next); - nlm_next = tmpptr; - } - *new_nlm_list = NULL; - - if (__libc_use_alloca (2 * this_buf_size)) - buf = extend_alloca (buf, this_buf_size, 2 * this_buf_size); - else - { - this_buf_size *= 2; - - char *new_buf = realloc (use_malloc ? buf : NULL, this_buf_size); - if (new_buf == NULL) - goto out_fail; - buf = new_buf; - - use_malloc = true; - } - buf_size = this_buf_size; - - iov.iov_base = buf; - iov.iov_len = this_buf_size; - - /* Increase sequence number, so that we can distinguish - between old and new request messages. */ - h->seq++; - - if (__netlink_sendreq (h, type) < 0) - goto out_fail; - - continue; - } + goto out_fail; size_t count = 0; size_t remaining_len = read_len; @@ -237,36 +195,6 @@ __netlink_request (struct netlink_handle struct nlmsgerr *nlerr = (struct nlmsgerr *) NLMSG_DATA (nlmh); if (nlmh->nlmsg_len < NLMSG_LENGTH (sizeof (struct nlmsgerr))) errno = EIO; - else if (nlerr->error == -EBUSY - && orig_this_buf_size != this_buf_size) - { - /* If EBUSY and MSG_TRUNC was seen, try again with a new - netlink socket. */ - struct netlink_handle hold = *h; - if (__netlink_open (h) < 0) - { - *h = hold; - goto out_fail; - } - __netlink_close (&hold); - orig_this_buf_size = this_buf_size; - nlm_next = *new_nlm_list; - while (nlm_next != NULL) - { - struct netlink_res *tmpptr; - - tmpptr = nlm_next->next; - free (nlm_next); - nlm_next = tmpptr; - } - *new_nlm_list = NULL; - count = 0; - h->seq++; - - if (__netlink_sendreq (h, type) < 0) - goto out_fail; - break; - } else errno = -nlerr->error; goto out_fail; Jakub