From owner-svn-src-head@freebsd.org Fri Feb 17 07:16:00 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A77CECE270D; Fri, 17 Feb 2017 07:16:00 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 562CD18EE; Fri, 17 Feb 2017 07:16:00 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id 7F184D67110; Fri, 17 Feb 2017 18:15:52 +1100 (AEDT) Date: Fri, 17 Feb 2017 18:15:52 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Gleb Smirnoff cc: Eric van Gyzen , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r313821 - in head/sys: dev/cxgb/ulp/iw_cxgb fs/nfsserver kern netinet netinet/libalias netpfil/ipfw In-Reply-To: <20170217041748.GI58829@FreeBSD.org> Message-ID: <20170217170724.B1386@besplex.bde.org> References: <201702162047.v1GKlf9j014479@repo.freebsd.org> <20170217041748.GI58829@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=VbSHBBh9 c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=ZI_ac5DnKVBrprJGNNMA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Feb 2017 07:16:00 -0000 On Thu, 16 Feb 2017, Gleb Smirnoff wrote: > heh, things are worse. Multiple places you changes are CTR() > macros. Neither inet_ntoa() nor inet_ntoa_r() will work with > them. :( > > Basicly non-constant strings can not be logged with KTR. All > the lines you touched should log actual binary value of the > IPv4 address. > > I am not asking you to do this work! :) But I couldn't leave > that without a comment. Nothing in KTR printing (exept %% and literals) works right. %s is only works for strings in kernel memory (fetched by kread() in ktrdump(8)). All args, even for the string pointers for %s, have type u_long. ktrdump doesn't check or translate formats, so all format descriptors except %lu (possibly qualified) except %lu give undefined behaviour that mostly works. ktr_tracepoint() only takes (printing) args of type u_long, and CTR6() casts to this. These casts inhibit type checking, and printf format is defeated by the use of non-const strings when ktrdump sees the format and by not using a variadic printf-like function for ktr_tracepoint(). > On Thu, Feb 16, 2017 at 08:47:41PM +0000, Eric van Gyzen wrote: > E> Author: vangyzen > E> Date: Thu Feb 16 20:47:41 2017 > E> New Revision: 313821 > E> URL: https://svnweb.freebsd.org/changeset/base/313821 > E> > E> Log: > E> Use inet_ntoa_r() instead of inet_ntoa() throughout the kernel > E> > E> inet_ntoa() cannot be used safely in a multithreaded environment > E> because it uses a static local buffer. Instead, use inet_ntoa_r() > E> with a buffer on the caller's stack. This is a lot of churn. _r functions are hard to use. If inet_ntoa() had not already existed, then someone should have invented it. It used to be a reasonable hack for functions that return static buffers to use an array of such buffers, with each caller getting a unique buffer unless the calls arrive at a high rate or the buffers remain in use for too long. This is not good enough with threads and/or preemption. The kernel inet_ntoa() didn't even do that. A good enough inet_ntoa() is easy to implement using alloca(). Something like: #define inet_ntoa(in) __extension__ ({ \ char *__cp; \ \ __cp = alloca(16); \ inet_ntoa_r(in, __cp, 16); \ }) I don't know how to do this using VLAs. The allocation must live until after leaving the the macro. alloc() maks it live until the end of the function. Kernel use must be careful not to allocate too much using alloca(), but this is not much harder than using explicit auto declarations. Compilers usually pessimize the auto declarations for space, so their space is not overlapped or reclaimed until the end of the function. alloca() only creates garbage at a higher rate if it is used in a loop. Converting to use malloc() would be worse than using auto declarations, since malloc() is slower and messier to clean up, though the actual malloc() can be hidden in a macro like the above. Implementations of alloca() on to of malloc() try to do garbage collection on function return (really on the next call to alloca(), if it detects that results of previous call(s) are garbage by looking at magic related to the calls). The latter is related to the simpler implementation with an array of static buffers. Even more simply and portably, I think it works to use a per-thread buffer: char td_msgbuf[32]; /* Buffer for small messages. */ ... #define td_alloc_msg(size) ... #define inet_ntoa(in) inet_ntoa_r(in, td_alloc_msg(16), 16) where td_alloc_msg(size) has to allocate 'size' bytes in curthread->td_msbug, circularly, starting at the previous position. This is the implementation using a static array, except I made td_msgbuf[64] generic so that it can be used for other functions with small buffer sizes other than 16. The sub-buffers are unique for each thread provided too many of them aren't used in a single statement. 64 allows just 2 buffers of size 16 for use in code like: printf("src %s, dst %s\n", inet_ntoa(src), inet_ntoa(dst)); You can also have 2 active assignments of inet_ntoa(any) at a time. If you want more, then expand the 32. This is already easier to use than the old inet_ntoa(9) -- using that required the discipline of only having 1 active assignment at a time, and using a UP && !PREEMPTION kernel to avoid races. Even more simpler to implement, but harder to use: char td_inet_ntoa_buf[16]; ... #define inet_ntoa(in) inet_ntoa_r(in, curthread->td_inet_ntoa_buf, 16) Callers now have to copy the result if they want to use more than 1 result. Old code already did this. Bruce