Maintain qualifiers on (struct thread_lite) members

[jtl]

Summary

Some of the members in struct thread_lite are missing qualifiers from struct thread. In the case of the volatile keyword, this can be important as it provides important information to the compiler about which assumptions it needs to follow when dealing with the data in the field.

Modify struct thread_lite to maintain the qualifiers from struct thread.

Test Plan

I reviewed the offsets.inc generated file and it appears correct.
I compiled a kernel and confirmed that kgdb shows the correct variable types and offsets.

[kib]

I definitely agree with the genoffset.sh changes.

On the other hand, I am not sure that we need to restore volatile qualifiers in the thread_lite, instead of removing them in the struct thread. The manipulations of the critnest level and pin count should use barriers, and I believe that the thread_lite patch and some of its follow-ups just did that. Volatile relied on the compiler-specific semantic to get similar effect.

[jtl]

In D17604#375736, @kib wrote:

I definitely agree with the genoffset.sh changes.

On the other hand, I am not sure that we need to restore volatile qualifiers in the thread_lite, instead of removing them in the struct thread. The manipulations of the critnest level and pin count should use barriers, and I believe that the thread_lite patch and some of its follow-ups just did that. Volatile relied on the compiler-specific semantic to get similar effect.

Thanks for the review!

Here are my thoughts:

1. struct thread and struct thread_lite should use the same qualifiers. (Actually, I think they should use exactly the same types.) That way, we end up with the same behavior regardless of which is used.
2. I do think there is a use for the volatile keyword, even with compiler barriers.

I could be wrong, but my understanding is that the volatile keyword keeps the compiler from assuming it can cache memory values in registers. Therefore, to increment a volatile variable, it would need to do something like this:

inc 0x48(%rbx)

Without the volatile qualifier, I think the following would be legal:

mov 0x48(%rbx), %rax
inc %rax
mov %rax, 0x48(%rbx)

If the value in memory changes between the load and store, it would be lost.

Therefore, if my understanding is correct, I think we need the volatile keyword on anything which either another thread or a hardware interrupt could change.

3. In this case, I believe a hardware interrupt can change either td_critnest or td_owepreempt. I believe the hardware interrupt should always restore td_critnest to its original value upon returning. Therefore, its probably OK if a hardware interrupt arrived in between a load and store of td_critnest. On the other hand, I believe a hardware interrupt can change the state of td_owepreempt. If the compiler loads this value, makes decisions, and then restores it, it is possible we would either miss a preempt or we would preempt unnecessarily. So, based on this reasoning, I believe we could drop the volatile qualifier for td_critnest, but not td_owepreempt.

I'd rather separate the two changes. If you agree with my analysis, I will open a separate review to drop the qualifier for td_critnest.

[kib]

In D17604#375787, @jtl wrote:

In D17604#375736, @kib wrote:

I definitely agree with the genoffset.sh changes.

On the other hand, I am not sure that we need to restore volatile qualifiers in the thread_lite, instead of removing them in the struct thread. The manipulations of the critnest level and pin count should use barriers, and I believe that the thread_lite patch and some of its follow-ups just did that. Volatile relied on the compiler-specific semantic to get similar effect.

Thanks for the review!

Here are my thoughts:

1. struct thread and struct thread_lite should use the same qualifiers. (Actually, I think they should use exactly the same types.) That way, we end up with the same behavior regardless of which is used.
2. I do think there is a use for the volatile keyword, even with compiler barriers.

I could be wrong, but my understanding is that the volatile keyword keeps the compiler from assuming it can cache memory values in registers. Therefore, to increment a volatile variable, it would need to do something like this:

inc 0x48(%rbx)

Without the volatile qualifier, I think the following would be legal:

mov 0x48(%rbx), %rax
inc %rax
mov %rax, 0x48(%rbx)

If the value in memory changes between the load and store, it would be lost.

I do not believe that volatile makes such guarantees. I think that gcc and clang only do not allow the accesses to vanish, i.e. to optimize them based on cached values. But the load/inc/store code generation is legal for volatile x++;. In fact, the standard states that the value in the externally visible storage must be correct on the sequence point, thats all.
See the handling of amd64 pcb_flags where we do care about the interrupt safety of the value and use inline assembly instead of relying on the code generation.

Another point, I do not think that we ever support returning from interrupts or exceptions with changed critnest or pin. We do allow using critical sections inside the interrupt handlers, but the entry must be balanced with the exit. The only value that can be changed by the interrupt handler is td_owepreempt.

Therefore, if my understanding is correct, I think we need the volatile keyword on anything which either another thread or a hardware interrupt could change.

3. In this case, I believe a hardware interrupt can change either td_critnest or td_owepreempt. I believe the hardware interrupt should always restore td_critnest to its original value upon returning. Therefore, its probably OK if a hardware interrupt arrived in between a load and store of td_critnest. On the other hand, I believe a hardware interrupt can change the state of td_owepreempt. If the compiler loads this value, makes decisions, and then restores it, it is possible we would either miss a preempt or we would preempt unnecessarily. So, based on this reasoning, I believe we could drop the volatile qualifier for td_critnest, but not td_owepreempt.

I'd rather separate the two changes. If you agree with my analysis, I will open a separate review to drop the qualifier for td_critnest.

Ok. As I said, the current changes, esp. to the generator script, are fine. I think that you can commit whole patch right now, but then it makes sense to remove excessive volatiles.