Date: Sat, 20 May 2006 04:08:55 GMT From: John Birrell <jb@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 97485 for review Message-ID: <200605200408.k4K48tRv093924@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=97485 Change 97485 by jb@jb_freebsd2 on 2006/05/20 04:07:55 Address a potential problem reported by Sun where a cache line on i386 might split the jmp instruction from the jmp offset, causing the two cache lines to become inconsistent. Instead of trying to replace two bytes with the jmp and offset, force a jmp into the generated code with a zero offset (meaning jump to the next address) and allow just the offset to be tweaked. I am still trying to avoid creating an invalid opcode trap like Solaris does just so that the enabled probe can be processed. I agree that is necessary for the fbt provider, but for the sdt provider we can choose what code we generate and we should be able to cater for both cases (enabled/disabled) witout taking a big performance hit by taking an invalid opcode trap. This change has no effect on the disabled probe performance. It is likely to still be just 3 clock cycles (depending on processor). An enabled probe, though, takes a further hit of those 3 clock cycles. This will probably not be noticed because firing a probe is a big clock cycle consumer. I am willing to bet that this will perform better on FreeBSD than on Solaris. If I can prove that, do I win an ipod? And to prove it I'd need to find a computer that will successfully run Solaris. I sit here surrounded by computers and not a single one will run Solaris. Sigh. Affected files ... .. //depot/projects/dtrace/src/sys/cddl/dev/sdt/sdt.c#2 edit .. //depot/projects/dtrace/src/sys/kern/kern_sdt.c#2 edit .. //depot/projects/dtrace/src/sys/sys/sdt.h#2 edit Differences ... ==== //depot/projects/dtrace/src/sys/cddl/dev/sdt/sdt.c#2 (text+ko) ==== @@ -137,7 +137,7 @@ /* * Point to the instruction after the start probe label. */ - p = (u_int16_t *) ref->probe_start; + p = (u_int16_t *) ref->probe_start + 1; /* * Enable the probe. @@ -160,7 +160,7 @@ /* * Point to the instruction after the start probe label. */ - p = (u_int16_t *) ref->probe_start; + p = (u_int16_t *) ref->probe_start + 1; /* * Disable the probe. ==== //depot/projects/dtrace/src/sys/kern/kern_sdt.c#2 (text+ko) ==== @@ -77,7 +77,7 @@ char probe_name[256]; struct sdt_ref *ref = arg; #if defined(__i386__) - u_int16_t *p; + u_int8_t *p; u_long offset; #endif @@ -145,19 +145,30 @@ /* * Point to the instruction after the start probe label. */ - p = (u_int16_t *) ref->probe_start; + p = (u_int8_t *) ref->probe_start; + + if (*p != 0xeb) { + printf("%s:%s:%s expected jmp (0xeb) as a relative jump to bypass the SDT probe.\n", ref->mod, ref->func, ref->name); + ref->state = SDT_UNEXPECTED_INSTR; + return; + } + + /* + * Point to the offset field. + */ + p++; /* - * Save the two instructions as the start of the probe. These are + * Save the two instruction as the start of the probe. This is * used when the probe is enabled later. */ ref->probe_enable = *p; /* - * Choose the two instructions that will disable the probe by - * jumping to the end probe label. + * Choose the off set that will disable the probe by jumping to the + * end probe label. */ - ref->probe_disable = (offset << 8) | 0xeb; + ref->probe_disable = offset; /* * Disable the probe. ==== //depot/projects/dtrace/src/sys/sys/sdt.h#2 (text+ko) ==== @@ -52,7 +52,8 @@ typedef enum { SDT_UNINIT = 1, SDT_INIT, - SDT_OFFSET_TOO_BIG + SDT_OFFSET_TOO_BIG, + SDT_UNEXPECTED_INSTR } sdt_state_t; struct sdt_ref { @@ -67,8 +68,8 @@ caddr_t probe_start; /* Start probe address. */ caddr_t probe_end; /* End probe address. */ #if defined(__i386__) - u_int16_t probe_enable; /* Instructions to enable the probe. */ - u_int16_t probe_disable; /* Instructions to disable the probe. */ + u_int8_t probe_enable; /* Instruction to enable the probe. */ + u_int8_t probe_disable; /* Instruction to disable the probe. */ #endif }; @@ -82,6 +83,7 @@ SYSUNINIT(ref##_sdt_uninit, SI_SUB_KDTRACE, SI_ORDER_ANY, \ sdt_deregister, &ref##_sdt_ref); \ SDT_LABEL(__##ref##_sdt_probe_start); \ + __asm__("jmp 0f; 0:"); \ if (ref##_sdt_ref.id != 0) \ (*sdt_probe_func)(ref##_sdt_ref.id, (uintptr_t) arg0, (uintptr_t) arg1, \ (uintptr_t) arg2, (uintptr_t) arg3 , (uintptr_t) arg4); \
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200605200408.k4K48tRv093924>