make critical_{enter, exit} inline

[mmacy]

Summary

more ../build/usr/home/mmacy/devel/freebsd/amd64.amd64/sys/GENERIC/offset.inc

#ifndef _OFFSET_INC_
#define _OFFSET_INC_
struct thread_lite {
        u_char  pad_td_epochnest[0xf9 - 0];
        u_char  td_epochnest;
        u_char  pad_td_owepreempt[0x120 - (0xf9 + sizeof(u_char))];
        u_char  td_owepreempt;
        u_char  pad_td_pinned[0x154 - (0x120 + sizeof(u_char))];
        int     td_pinned;
        u_char  pad_td_priority[0x392 - (0x154 + sizeof(int))];
        u_char  td_priority;
        u_char  pad_td_pre_epoch_prio[0x396 - (0x392 + sizeof(u_char))];
        u_char  td_pre_epoch_prio;
        u_char  pad_td_critnest[0x48c - (0x396 + sizeof(u_char))];
        int     td_critnest;
};
#endif

[mmacy]

add new files

[jeff]

sys/kern/genoffset.c

37 ↗

(On Diff #44694)

As mentioned elsewhere, we should get the type from typeof().

sys/kern/genoffset.sh

2 ↗

(On Diff #44694)

Maybe we should have a license?

57 ↗

(On Diff #44694)

I should've commented this. It's more complicated than it could've been if we had gawk extensions.

sys/sys/thread_lite.h

33 ↗

(On Diff #44694)

If you want to do one file per-type we should have the generator work that way. This will be weird as soon as you add another structure. I had imagined there would be a sys/kpilite.h or something with them all but I don't mind either way. the awk can be changed to generate multiple files or we can have multiple GENOFF template files.

[mmacy]

incorporate feedback

[mmacy]

sys/kern/genoffset.c

37 ↗

(On Diff #44694)

Yes. If you can determine how to do that.

[kib]

Type info can be recovered from the .o compiled with -g, using dwarf dump utilities, which we do not have in base.

When we discussed inlining critical_enter(9) with mjg, my opinion was that struct thread_lite only adds complications. It is good enough to only have the offsets to the members auto-generated, and manually calculate the addresses of the td_critnest and td_owepreempt. It seems that I am the only one who thinks so, everybody else prefer thread_lite. genoffset.h is the good illustration of what I mean. BTW, what are the restrictions on the structure definitions which are processed by the script ?

sys/sys/systm.h

52 ↗

(On Diff #44695)

What is kpilite.h ?

It is quite unfortunate to add new pollution to sys/systm.h. Why is it needed ?

[mmacy]

In D16078#340860, @kib wrote:

Type info can be recovered from the .o compiled with -g, using dwarf dump utilities, which we do not have in base.

When we discussed inlining critical_enter(9) with mjg, my opinion was that struct thread_lite only adds complications. It is good enough to only have the offsets to the members auto-generated, and manually calculate the addresses of the td_critnest and td_owepreempt. It seems that I am the only one who thinks so, everybody else prefer thread_lite. genoffset.h is the good illustration of what I mean. BTW, what are the restrictions on the structure definitions which are processed by the script ?

That ends up being quite ugly. The use of macros here is the least of the issues here:

#define CRITICAL_ENTER(td)										\
	do {														\
		int *td_critnest = (int *)(((char *)td) + TD_CRITNEST);	\
		(*td_critnest)++;										\
		__compiler_membar();									\
	} while (0)
#define CRITICAL_EXIT(td)												\
	do {																\
	int *td_critnest = (int *)(((char *)td) + TD_CRITNEST);				\
	u_char *td_owepreempt = (u_char *)(((char *)td) + TD_OWEPREEMPT);	\
	MPASS(*td_critnest > 0);											\
	__compiler_membar();												\
	(*td_critnest)--;													\
	__compiler_membar();												\
	if (__predict_false(*td_owepreempt != 0))							\
		critical_exit_preempt();										\
} while (0)
#define SCHED_PIN(td)											\
	do {														\
		int *td_pinned = (int *)(((char *)td) + TD_PINNED);		\
		(*td_pinned)++;											\
		__compiler_membar();									\
	} while (0)

#define SCHED_UNPIN(td)											\
	do {														\
		int *td_pinned = (int *)(((char *)td) + TD_PINNED);		\
		__compiler_membar();									\
		MPASS(*td_pinned > 0);									\
		(*td_pinned)--;											\
		__compiler_membar();									\
	} while (0)

static __inline void
epoch_enter_preempt(epoch_t epoch, epoch_tracker_t et)
{
	struct epoch_record *er;
	struct epoch_thread *etd;
	caddr_t ptd;
	u_char *td_priority, *td_pre_epoch_prio;
	u_char *td_epochnest;
	MPASS(cold || epoch != NULL);
	INIT_CHECK(epoch);
	etd = (void *)et;
#ifdef INVARIANTS
	MPASS(epoch->e_flags & EPOCH_PREEMPT);
	etd->et_magic_pre = EPOCH_MAGIC0;
	etd->et_magic_post = EPOCH_MAGIC1;
#endif
	ptd = (caddr_t)curthread;
	etd->et_td = (void*)ptd;
	td_epochnest = ptd + TD_EPOCHNEST;
	(*td_epochnest)++;
	CRITICAL_ENTER(ptd);
	SCHED_PIN(ptd);

	td_priority = (u_char *)ptd + TD_PRIORITY;
	td_pre_epoch_prio = (u_char *)ptd + TD_PRE_EPOCH_PRIO;
	*td_pre_epoch_prio = *td_priority;
	er = epoch->e_pcpu[curcpu];
	TAILQ_INSERT_TAIL(&er->er_tdlist, etd, et_link);
	ck_epoch_begin(&er->er_record, (ck_epoch_section_t *)&etd->et_section);
	CRITICAL_EXIT(ptd);
}

Most would agree this is much nicer:

/*
 * Standalone (_sa) routines for thread state manipulation
 */
static __inline void
critical_enter_sa(void *tdarg)
{
	struct thread_global *td;

	td = tdarg;
	td->td_critnest++;
	__compiler_membar();
}

static __inline void
critical_exit_sa(void *tdarg)
{
	struct thread_global *td;

	td = tdarg;
	MPASS(td->td_critnest > 0);
	__compiler_membar();
	td->td_critnest--;
	__compiler_membar();
	if (__predict_false(td->td_owepreempt != 0))
		critical_exit_preempt();
}

static __inline void
sched_pin_sa(void *tdarg)
{
	struct thread_global *td;

	td = tdarg;
	td->td_pinned++;
	__compiler_membar();
}

static __inline void
sched_unpin_sa(void *tdarg)
{
	struct thread_global *td;

	td = tdarg;
	MPASS(td->td_pinned > 0);
	__compiler_membar();
	td->td_pinned--;
	__compiler_membar();
}

static __inline void
epoch_enter_preempt(epoch_t epoch, epoch_tracker_t et)
{
	struct epoch_record *er;
	struct epoch_thread *etd;
	struct thread_global *td;
	MPASS(cold || epoch != NULL);
	INIT_CHECK(epoch);
	etd = (void *)et;
#ifdef INVARIANTS
	MPASS(epoch->e_flags & EPOCH_PREEMPT);
	etd->et_magic_pre = EPOCH_MAGIC0;
	etd->et_magic_post = EPOCH_MAGIC1;
#endif
	td = (struct thread_global *)curthread;
	etd->et_td = (void*)td;
	td->td_epochnest++;
	critical_enter_sa(td);
	sched_pin_sa(td);

	td->td_pre_epoch_prio = td->td_priority;
	er = epoch->e_pcpu[curcpu];
	TAILQ_INSERT_TAIL(&er->er_tdlist, etd, et_link);
	ck_epoch_begin(&er->er_record, (ck_epoch_section_t *)&etd->et_section);
	critical_exit_sa(td);
}

sys/sys/systm.h

52 ↗

(On Diff #44695)

kpilite.h conditionally includes offset.inc which is necessary for the inline critical definitions. We don't have to include it but then critical_{enter, exit} would only be inline for files that explicitly included it. If we're seeking to avoid pollution there are things that are likely higher priority (see seq.h ending up being included everywhere for example).

[imp]

Rather than (int *)((char *)td + offset), you might need to do (int *)((uinptr_t)td + offset) to get around alignment changed warnings.

[imp]

so you have both thread_lite and offset generator... Why?

[jeff]

In D16078#340968, @imp wrote:

so you have both thread_lite and offset generator... Why?

thread_lite is generated by the awk.

I would like it if we either generated a verification file that is built into the kernel that asserts typeof and offsetof matches. Or if we can easily extract type information as kib suggested. I like this approach in general but I just want to avoid the inevitable bugs that will come from any mismatch.

[jeff]

In D16078#340860, @kib wrote:

Type info can be recovered from the .o compiled with -g, using dwarf dump utilities, which we do not have in base.

When we discussed inlining critical_enter(9) with mjg, my opinion was that struct thread_lite only adds complications. It is good enough to only have the offsets to the members auto-generated, and manually calculate the addresses of the td_critnest and td_owepreempt. It seems that I am the only one who thinks so, everybody else prefer thread_lite. genoffset.h is the good illustration of what I mean. BTW, what are the restrictions on the structure definitions which are processed by the script ?

I understand your hesitation. I like it if we can make it more strongly verified. Managing offsets is really only palatable if we just use it for this one feature. I suspect this will give us opportunities to unwind some headers a bit and it may see more use.

The awk script is pretty dumb. It just takes the two types as strings. If the member type is complex it will need its header included in the generated file which somewhat defeats the purpose. The code uses sizeof to calculate the padding.

[mmacy]

• assert that sizeof field matches sizeof datatype

[kib]

In D16078#340965, @mmacy wrote:

When we discussed inlining critical_enter(9) with mjg, my opinion was that struct thread_lite only adds complications. It is good enough to only have the offsets to the members auto-generated, and manually calculate the addresses of the td_critnest and td_owepreempt. It seems that I am the only one who thinks so, everybody else prefer thread_lite. genoffset.h is the good illustration of what I mean. BTW, what are the restrictions on the structure definitions which are processed by the script ?

That ends up being quite ugly. The use of macros here is the least of the issues here:

...

Most would agree this is much nicer:

Yes, I understand the prevailing opinion, I just wanted to express mine. It is clear that thread_lite version is going to be used.

sys/sys/systm.h

52 ↗

(On Diff #44695)

Why cannot the ifdef wrapper put into the generated offset.inc ?

[mmacy]

The awk script is pretty dumb. It just takes the two types as strings. If the member type is complex it will need its header included in the generated file which somewhat defeats the purpose. The code uses sizeof to calculate the padding.

genoffset.c will fail to compile if the size of the declared doesn't match. It doesn't address sign issues. But I think it provides the safety guarantees we need.

[mmacy]

sys/sys/systm.h

52 ↗

(On Diff #44695)

No reason. I'll do that.

[mmacy]

• include offset.inc directly in systm.h
• define sched_{pin, unpin} variant that doesn't depend on proc.h

[mmacy]

• validate type and offset
• fix circular dependency by reincluding kpilite.h
• move include down below KASSERT definition

[mmacy]

• actually compile in offset checks

[jeff]

sys/kern/genoffset.sh

5 ↗

(On Diff #44743)

This should probably be me and whoever checked in the original genassym.sh