<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof ContentPasted0">
<div><span style="">I see this v2 patch in the mail archives, even though it never showed up in my inbox. Ah well.</span><br>
</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">Reviewed-by: Eugene Loh <eugene.loh@oracle.com></div>
<div class="ContentPasted0">though there is some strsize weird thing noted below.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">==== bpf/inet_ntoa6.S</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">Replace every instance of 0x3a with ':'.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">Why the simple copyright banner? No reference to Oracle Linux DTrace or UPL?</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">Thanks for the comment</div>
<div class="ContentPasted0"> * We sadly cannot include struct declarations in assembler input files, so we</div>
<div class="ContentPasted0"> * cannot use offsetof() to programmatically determine the offset of the rodata</div>
<div class="ContentPasted0"> * member in the DTrace context (dt_ctx_t). If that structure changes, the</div>
<div class="ContentPasted0"> * following define must be updated as well.</div>
<div class="ContentPasted0">but is there somewhere in dt_dctx.h where we can reference DCTX_RODATA to make this potential problem easier to find? E.g., where "typedef struct dt_dctx {...} dt_dctx_t;" is defined, insert a comment that if the offset to rodata
is changed, then the hard-coded value of DCTX_RODATA in bpf/inet_ntoa6.S needs to be updated.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">There is the write_hex16 block comment:</div>
<div class="ContentPasted0"> * Let s (%r0) be the number of digits output thus far. It should be</div>
<div class="ContentPasted0"> * incremented if it is non-zero or if the current digit is non-zero,</div>
<div class="ContentPasted0"> * which can be expressed as (s + c) > 0. We only advance the output</div>
<div class="ContentPasted0"> * pointer if (s + c) > 0.</div>
<div class="ContentPasted0"> *</div>
<div class="ContentPasted0"> * To avoid branches, we calculate ((-(c + s)) >> 63) as the value to</div>
<div class="ContentPasted0"> * add to the output pointer (and to s), becuse its value will be 0 iff</div>
<div class="ContentPasted0"> * c and s are both 0, and 1 otherwise.</div>
<div class="ContentPasted0">At least, s/becuse/because/.</div>
<div class="ContentPasted0">But how about actually reducing the whole thing to:</div>
<div class="ContentPasted0"> * Let s (%r0) be the number of digits output thus far. It should be</div>
<div class="ContentPasted0"> * incremented if it is non-zero or if the current digit c is non-zero.</div>
<div class="ContentPasted0"> * This is achieved without branches by adding ((-(c + s)) >> 63) to</div>
<div class="ContentPasted0"> * the output pointer (and to s).</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">Also in write_hex16, I see:</div>
<div class="ContentPasted0"> add %r3, %r0 /* %r3 = ((-(c + s)) >> 63) */</div>
<div class="ContentPasted0"> neg %r3</div>
<div class="ContentPasted0"> rsh %r3, 63 /* 0 if '0', 1 otherwise */</div>
<div class="ContentPasted0">What is '0'? An ASCII char? How about just skipping that comment, since this distinctive construct was just explained.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">Thanks for documenting</div>
<div class="ContentPasted0"> * %r9 dst</div>
<div class="ContentPasted0"> * %r8 src</div>
<div class="ContentPasted0"> * %r7 bitmap of non-zero words</div>
<div class="ContentPasted0"> * %r6 dctx</div>
<div class="ContentPasted0"> * [%fp-8] tbloff</div>
<div class="ContentPasted0">but how about</div>
<div class="ContentPasted0"> * %r9 output pointer</div>
<div class="ContentPasted0"> * %r8 input pointer</div>
<div class="ContentPasted0"> * %r7 bitmap of non-zero words</div>
<div class="ContentPasted0"> * %r6 dctx (ipv4 code path), temp reg (ipv6 code path)</div>
<div class="ContentPasted0"> * [%fp-4] tbloff</div>
<div class="ContentPasted0">Note the s/8/4/ for tbloff.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">I see:</div>
<div class="ContentPasted0"> * We make use of the fact that dst is a tstring which is known to be</div>
<div class="ContentPasted0"> * large enough to hold the longest output (OUTPUT_LEN = 40 bytes), and</div>
<div class="ContentPasted0"> * two copies of the input data (2 * INPUT_LEN = 32 bytes).</div>
<div class="ContentPasted0">How do we know a tstring is large enough? In fact, I see</div>
<div class="ContentPasted0"> $ dtrace -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l</div>
<div class="ContentPasted0"> 8</div>
<div class="ContentPasted0"> $ dtrace -xstrsize=512 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l</div>
<div class="ContentPasted0"> 8</div>
<div class="ContentPasted0"> $ dtrace -xstrsize=288 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l</div>
<div class="ContentPasted0"> 8</div>
<div class="ContentPasted0"> $ dtrace -xstrsize=256 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l</div>
<div class="ContentPasted0"> 8</div>
<div class="ContentPasted0"> $ dtrace -xstrsize=240 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l</div>
<div class="ContentPasted0"> 67117</div>
<div class="ContentPasted0"> $ dtrace -xstrsize=224 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l</div>
<div class="ContentPasted0"> 67117</div>
<div class="ContentPasted0"> $ dtrace -xstrsize=192 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l</div>
<div class="ContentPasted0"> 67117</div>
<div class="ContentPasted0"> $ dtrace -xstrsize=128 -s test/unittest/funcs/tst.inet_ntoa6.d |& wc -l</div>
<div class="ContentPasted0"> 67117</div>
<div class="ContentPasted0">though presumably something else is going on other than just 40+2*16>strsize.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">The rationale</div>
<div class="ContentPasted0"> * The advantage of keeping the original copy</div>
<div class="ContentPasted0"> * around is that we can use the unconverted words as input to</div>
<div class="ContentPasted0"> * inet_ntoa() for mapped or embedded IPv4 addresses.</div>
<div class="ContentPasted0">is a little funny since the original src could serve just as well. I guess keeping src would consume another register, though. Anyhow, how about just</div>
<div class="ContentPasted0"> * The unconverted copy is retained to pass to dt_inet_ntoa.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">In the comments, I'd avoid the labels "copy1" and "copy2". They don't otherwise appear. Plus, "copy1" is the second copy, while "copy2" is the first copy, which is kind of confusing. How about "converted copy" and "unconverted
copy," respectively.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">In</div>
<div class="ContentPasted0"> * The exact semantics are a bit fuzzy in that neither RFC 4291 nor</div>
<div class="ContentPasted0"> * RFC 5952 address the case where a 6 zero-words are followed by 2</div>
<div class="ContentPasted0"> * words that do not form a valid IPv4 address. Legacy DTrace takes</div>
<div class="ContentPasted0"> * the interpretation that a 6 zero-word prefix indicates an IPv4</div>
<div class="ContentPasted0"> * IPv4-compatible IPv6 address, whereas e.g. glibc requires that the</div>
<div class="ContentPasted0"> * last 2 words form a valid IPv4 address (i.e. first octet cannot be</div>
<div class="ContentPasted0"> * zero).</div>
<div class="ContentPasted0">the "IPv4" at the end of the fourth line looks redundant.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">I see:</div>
<div class="ContentPasted0"> * The implementation here adopts the legacy approach (in order):</div>
<div class="ContentPasted0">The legacy implementation relies on __ipv6_addr_type(), which I *THINK* uses a slightly different order than what I see in bpf/inet_ntoa6.S, but I guess that's no big deal.</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">s/paarts/parts/</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">In</div>
<div class="ContentPasted0"> /*</div>
<div class="ContentPasted0"> * Determine the number of words to output at the start of the address.</div>
<div class="ContentPasted0"> * It is found in the upper 4 bits in %r7 (result of the table lookup</div>
<div class="ContentPasted0"> * above). We place an upper bound to make the BPF verifier happy.</div>
<div class="ContentPasted0"> */</div>
<div class="ContentPasted0"> mov %r6, %r7</div>
<div class="ContentPasted0"> rsh %r6, 4</div>
<div class="ContentPasted0"> jgt %r6, 7, .Ldone</div>
<div class="ContentPasted0">how about replacing the branch with "and %r6, 7" instead. One less branch. After all, the point is to make the BPF verifier happy!</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">In</div>
<div class="ContentPasted0"> .Ldone:</div>
<div class="ContentPasted0"> /* Output the erminating NUL byte and return. */</div>
<div class="ContentPasted0">that should be "terminating".</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">In</div>
<div class="ContentPasted0"> .Lipv4:</div>
<div class="ContentPasted0"> /* Output the last two words as an IPv2 address and return. */</div>
<div class="ContentPasted0"> mov %r1, %r6</div>
<div class="ContentPasted0"> mov %r2, %r8</div>
<div class="ContentPasted0"> add %r2, 16 + 12</div>
<div class="ContentPasted0"> mov %r3, %r9</div>
<div class="ContentPasted0"> call dt_inet_ntoa</div>
<div class="ContentPasted0">how about</div>
<div class="ContentPasted0"> s/IPv2/IPv4/</div>
<div class="ContentPasted0"> s/16 + 12/INPUT_LEN + INPUT_LEN - 4/</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">==== libdtrace/dt_cg.c</div>
<div><br class="ContentPasted0">
</div>
<div class="ContentPasted0">I thought there were going to be some comments to explain the values in the table.</div>
<div><br class="ContentPasted0">
</div>
Actually, how about replacing those table values with code that generates the table? That *might* be more compact, but in any case it gives a precise definition of the table contents, helping readers both to understand and to check the values.<br>
</div>
</body>
</html>