[DTrace-devel] [PATCH] Implement the ip provider

[Eugene Loh]

I didn't get this patch in my inbox, so sorry if this looks like a 
weirdly formatted review.

I suppose I need to lead with this:  testing looks problematic. I'm 
going to guess you developed on something like x86/OL9. Testing results 
on other platforms are poor.  I found one of your branches and ran 
aed542fe427b origin/kvh/2.0-branch-dev-ip "Implement the ip provider" with
     test/demo/ip/ipio.d
     test/demo/ip/ipproto.d
     test/unittest/ip/tst.ipv4localicmp.sh
     test/unittest/ip/tst.ipv4localtcp.sh
     test/unittest/ip/tst.ipv4localudp.sh
     test/unittest/ip/tst.ipv4remoteicmp.sh
     test/unittest/ip/tst.ipv4remotetcp.sh
     test/unittest/ip/tst.ipv4remoteudp.sh
     test/unittest/ip/tst.ipv6localicmp.sh
     test/unittest/ip/tst.ipv6remoteicmp.sh
The results were:
     x86 OL7 UEK6  :10 cases (2 PASS, 5 FAIL, 1 XPASS, 2 XFAIL, 0 SKIP)
     arm OL7 UEK6  :10 cases (2 PASS, 5 FAIL, 1 XPASS, 2 XFAIL, 0 SKIP)
     x86 OL8 UEK6  :10 cases (0 PASS, 7 FAIL, 0 XPASS, 3 XFAIL, 0 SKIP)
     arm OL8 UEK6  :10 cases (0 PASS, 7 FAIL, 0 XPASS, 3 XFAIL, 0 SKIP)
     x86 OL8 UEK7  :10 cases (1 PASS, 6 FAIL, 1 XPASS, 2 XFAIL, 0 SKIP)
     arm OL8 UEK7  :10 cases (1 PASS, 6 FAIL, 1 XPASS, 2 XFAIL, 0 SKIP)
     x86 OL9 UEK7  :10 cases (8 PASS, 0 FAIL, 1 XPASS, 1 XFAIL, 0 SKIP)
     arm OL9 UEK7  :10 cases (1 PASS, 7 FAIL, 1 XPASS, 1 XFAIL, 0 SKIP)
More specifically (I hope this is decipherable):
     6 6 6 6 7 7 7 7 UEK
     7 7 8 8 8 8 9 9 OL
     x A x A x A x A x86 or ARM
     * *     * * * * test/demo/ip/ipio.d: XPASS (dtv2).
         * *         test/demo/ip/ipproto.d: FAIL: erroneous exitcode (1).
     * * * * * *   * test/unittest/ip/tst.ipv4localicmp.sh: FAIL: 
expected results differ.
     * * * * * *   * test/unittest/ip/tst.ipv4localtcp.sh: FAIL: 
expected results differ.
     * * * * * *   * test/unittest/ip/tst.ipv4localudp.sh: FAIL: 
expected results differ.
     * * * * * *   * test/unittest/ip/tst.ipv4remoteicmp.sh: FAIL: 
expected results differ.
     * * * * * *   * test/unittest/ip/tst.ipv6localicmp.sh: FAIL: 
expected results differ.
         * * * *   * test/unittest/ip/tst.ipv4remoteudp.sh: FAIL: 
erroneous exitcode (1).
                   * test/unittest/ip/tst.ipv4remotetcp.sh: FAIL: 
expected results differ.

The legacy implementation had two other probes:  drop-in and drop-out.  
If this implementation will not have them (yet?), that should be noted.

There seems to be no correspondence to the legacy DTRACE_IP() 
instrumentation points.  It would be nice to have a note about 
implementation.  How were the instrumentation points (fbt probes) 
chosen?  Is this related to __IP[6]_UPD_PO_STATS(..., IPSTATS_MIB_IN, 
...) and IP[6]_UPD_PO_STATS(..., IPSTATS_MIB_OUT, ...)?

Maybe this is not related to the patch, but there does not seem to be 
any documentation on the provider... not even in the legacy docs?  Not 
sure.  Anyhow, again, I suppose that's unrelated to this particular patch.

If the ip_addr_t problem is being fixed, maybe reference Orabug 34855291?

Instead of
         static int get_member(dt_pcb_t *pcb, const char *name, int reg,
                               const char *member) {
how about
         static int
         get_member(dt_pcb_t *pcb, const char *name, int reg, const char 
*member)
         {
That puts the line breaks in nicer locations and is more consistent with 
the predominant style.

Then, in get_member(), you have:

+    if (reg != BPF_REG_0)
+        emit(dlp, BPF_MOV_REG(BPF_REG_0, reg));
+
+    emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_FP));
+    emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_TRAMP_SP_BASE));
+    emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
+    emit(dlp, BPF_MOV_REG(BPF_REG_3, reg));
+    emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, ctm.ctm_offset / NBBY));
+    emit(dlp, 
BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
+    emit(dlp, BPF_LOAD(ldop, BPF_REG_0, BPF_REG_FP, DT_TRAMP_SP_BASE));

It makes no sense to %r0 = reg right before %r0 gets clobbered. (I 
imagine that's a vestigial instruction.)  Also, should the return code 
of probe_read be checked?  Incidentally I do something similar in the io 
provider.  I don't know if the two versions should be combined... in any 
case, it's not important.  Mine is constructed slightly differently due 
to some stuff going on there.  Arguably, both of us should be combining 
forces with dt_cg_load_scalar(), but maybe not since load_scalar() has 
its own idiosyncratic needs.

You say
+       /*
+        * We construct the ip:::(receive,send) probe arguments as follows:
+        *      args[0] = skb
+        *      args[1] = skb->sk
+        *      args[2] = ip_hdr(skb)
+        *      args[3] = skb->dev
+        *      args[4] = ip_hdr(skb)
+        *      args[5] = NULL
+        */
but the descriptions of args[4] and args[5] do not quite agree with what 
you do.