Please commit with "Approved by: jtl"

Has this landed yet?

Please commit with Approved by: jtl

By the way, in the commit message, please make it clear that this simple "allows" you to more easily use arc if you want your changes to be reviewed. It doesn't require it in any way, shape, or form. And, we're not changing any expectations regarding when someone will get review.

LGTM

I'm OK with importing the code as an option for developers to use. I'd be wary of specifying that there is a "plan" to convert existing test cases unless there is general buy in to do that.

Regardless of what the group says, it seems like we'll probably want to support both for a transitional period. Given that, this change seems OK. @emaste , do you agree?

Since make clean runs first, I'm assuming the only things left will be things with flags set? If so, what you propose is probably correct (and we don't need the rm/chflags/rm "dance" optimization).

LGTM; would appreciate also hearing @rrs's opinion.

In D17914#382797, @markj wrote:

I'm writing a regression test for this to stick under tests/sys/netinet. I'll see if I can add test cases for D17922 as well.

Yeah, I saw this shortly before seeing this review. I agree we should fix it.

If alignment is important, we can (theoretically) do something more intelligent to maintain it. However, I think this is both safe and good enough for an emergency fix.

In D17609#375841, @mjg wrote:

I think this is a very questionable idea. In my experience clang does *mostly* not re-read. For all cases you are adding a function call.

See vfs_ref/vfs_rel or fsetown as examples.

Can you show me a specific function which compiles to multiple reads of gs just to get curthread?

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.

Fix stupid syntax error.

In D17503#374613, @sbahra_repnop.org wrote:

Let's merge from upstream if possible.

In D17492#374619, @sbahra_repnop.org wrote:

Ok, call should be fine as well! In all cases, seems like writers are synchronized and any other usage is in the context of a protected section. CI also passes (though I haven't run it through SPARCv9+ and ARM, but we already have RMO and TSO coverage, nothing specific to ARM / SPARC with these changes).

Thanks again Jon. These are great.

Please make the two changes suggested by bz@ and then feel free to commit this. I think its OK to ask re@ to commit this during the freeze. (I also won't be upset if they say "no".)

In D17492#373625, @sbahra_repnop.org wrote:

Ok. I caught up with Matt offline. This doesn't solve the problem at hand. The API is not being used as intended in some areas, so we will work to address that. I would advise against merging this in as it doesn't address the underlying issues:

• Some usage of epoch records is re-entrant: This is not supported. Supporting this would require atomic usage that would be, expensive. We have an easy fix for that.

In D17503#373574, @sbahra_repnop.org wrote:

In D17503#373529, @jtl wrote:

In D17503#373490, @sbahra_repnop.org wrote:

See previous note in D17492 regarding the assumption that write-side functions are *not* called with-in a read-side critical. Does FreeBSD require that this is permitted?

See my comments in that review.

Regardless of whether that is true, it seems like this change would still be safe. Do you agree?

Is it expected by the kernel that ck_epoch write-side functions are safe to call on records that are already in a read-side critical section?
Helping me understand the intended use-case will allow me to answer accurately. There is further validation needed beyond poll.

In D17492#373575, @sbahra_repnop.org wrote:

The above change should not have any impact if ck_epoch is being correctly used. For that reason, it's good to go. However, I am concerned about a larger problem.

For barriers, we do assert that the caller is not in a read section. However, the CPU's record may still be in a read section. So, maybe we do violate the reader/writer assumption there, too.

As long as the CPU record does not require re-entrance support, that's fine. The requirement is that the record that is used to issue a write-side request is not in a read-side section. We can lift this restriction, but I'll need to validate a few things. Is the kernel violating this restriction at the moment? I had trouble understanding that from the example given, because if the tick handler has a dedicated record, there is no problem and then the above change is a no-op.

If the above constraint is being violated, then I'll get back to you tonight as I'll want to do some additional validation.

In D17503#373490, @sbahra_repnop.org wrote:

See previous note in D17492 regarding the assumption that write-side functions are *not* called with-in a read-side critical. Does FreeBSD require that this is permitted?

In D17492#373479, @sbahra_repnop.org wrote:

Thanks much for this. Note that the assumption here is a writer thread *should not* be calling ck_epoch_synchronize or write while inside of a read-side critical section. In other words, the active bit must be set to 0. In this situation, the invariant is not violated. If inside a read-side critical section, bets are off. Is the assumption or requirement here that it be safe to call synchronize from with-in a read-side critical section? If so, there are a few other areas we could fix up.

The other half of the original diff is now in D17503.

Split the commit into two parts. In this part, just fix the early epoch calls.

FYI, it looks like there was a typo in the description:

Looks good. Please commit with "Approved: jtl (mentor)".

Hi. I know I'm late to the party, but I have three comments:
a) I could be wrong, but I don't think there is any guarantee this won't be called simultaneously for two different groups at the same time. (The groups could be in different VNETs, for example.) In that case, two different invocations could be working on the function's static variables at the same time. That may produce unexpected results. (Granted, it would take an unusual series of events. But, I think we've all seen highly unusual events occur.)
b) I don't think the const variable also needs to be static.
c) It seems like the rate limiter should really be per-group, so I would suggest adding the lastprint variable to the inpcblbgroup struct.

FYI, something didn't look right doing my tests. So, I'm going to delay committing this until I can satisfy myself that it behaves correctly. That will almost certainly mean I miss the code freeze. C'est la vie!

• Limit the ipq structure to the kernel to eliminate a buildworld failure. (And, why should we make userspace code import the sys/queue.h header for a structure they don't need anyway?)
• Address @jhb's nit.

In D16471#357964, @j-nitrology.com wrote:

D16626 expanded on these changes and was recently committed. I believe we can close this one out.

LGTM (with minor change noted in-line).

In D16626#356917, @rrs wrote:

This version incorporates all of Jonathans comments and suggested improvements. Thanks
Jonathan!!!