In D21209#461161, @ae wrote:

If you know that the problem occurs only with BPF, bpfwrite() invokes if_output() already in epoch section.

Epoch, dude.

Nice. This overall looks like a pretty great idea to me.

This wasn't not making progress due to the blocking review, it was not making progress since the current diff probably can't land as is. It seems everyone is in favor of the listening socket stashing approach, but I just haven't looked into it yet.

In D14604#307002, @jtl wrote:

In D14604#306967, @mjoras wrote:

imp actually just added stating not to use it to the man page in r330371, is that good enough? :) I agree that it is basically a style problem, but I don't know how else we are supposed to catch these other than periodically grepping the tree.

IMHO, the man page should also state that there is a CTASSERT for it. And, the man page should also explain why this is serious enough that we are going to CTASSERT on style violations.

Ideally, this would be in some sort of linter that we encourage people to run before they commit. Or, you can make the CTASSERT only be active for LINT builds. Or, we can deal with it the same way we deal with most other style violations: peer pressure by emailing the offender after a commit.

A CTASSERT just seems like too heavy of a hammer for this. But, maybe I'm missing the importance?

In D14604#306835, @jtl wrote:

I agree with eliminating the pointless uses of the flag; however, I don't understand why we need a CTASSERT to catch this. It doesn't break anything. At most, it seems like it is simply a style violation. And, I don't think CTASSERTs are necessary to enforce style.

If you think there is a really good reason to add CTASSERTs, please update the man page to indicate this. At least, that will document it for developers (like me :-) ) who regularly add them out of habit.

Hans, did you ever get a chance to test this?

In D14547#306226, @eugen_grosbein.net wrote:

In D14547#306225, @rstone wrote:

In D14547#306224, @eugen_grosbein.net wrote:

Care to elaborate what are you trying to achieve by moving subnet route between interfaces?

My customer is using it as a lame form of failover. If the interface with address A goes down, we can fail over the subnet route to address B (on a different interface) and address B remains functional (address A remains down, of course. I did same it was a lame form of failover).

I have no ability to push back on the customer on this point. Their position is that this configuration was supported on previous versions and therefore must remain supported.

In which "previous version" was it "supported"?

This worked on older versions of our product, which were based roughly on FreeBSD 7.x

In D14385#301730, @ae wrote:

In D14385#301537, @mjoras wrote:

You can't just state it has "significant performance penalties" by looking at the code. Consider also that the path if_output takes isn't as direct as you might think anyway. When a vlan's if_output gets called you end up back in ether_output_frame. That ends up calling the interface's if_transmit, which puts you back in vlan_transmit. So the old path would have been:

Reducing inbound call path improves forwarding performance for up to 20%. Additional entries can hit performance. This is not noticeable for stock FreeBSD, that can't do more that 3-5Mpps due to lock contention. But when packet rate is about 10-12Mpps it will be significant. We can ask Olivier to try test with our patches.

In D14385#301524, @eugen_grosbein.net wrote:

Why don't we just reject creation of vlan interface on lagg member (as opposed to lagg itself)?

In D14385#301506, @melifaro wrote:

The issue which this change is trying to solve is control plane issue. Dealing with it in the data path code seem to be a wrong approach. Additionally, it imposes significant performance penalties.
The better way of doing this is to have a "solver" function which is able to handle such cases. Calls to this function can be triggered by ifnet/lagg change events subscription.

In D11725#298967, @ae wrote:

It would be good have some progress with this review.

Thanks for this. I've noticed this behavior when debugging an unrelated ND6 issue, but never dug into where it happens in the code.

In D14051#295069, @rgrimes wrote:

Only question I have before we remove this, is the tree void of code in this format?

If it is not then I think it would be unwise to remove this,if it is then yes, absolutely, remove this.

First off, please submit future patches with arc diff directly, or with a diff generated with full context.

• in order to be MFCable we can't change the eventhandler_list layout.

• style fixups.

• manpage style and extraneous use of "relative"
• remove the flags field as it is not needed.

• style nit

• use a pointer as the global reference to the pre-defined list, allowing for these lists to be created on-register (e.g. from a module) before they are defined. This was the approach suggested and taken by ian in his revision.
• refactor API usage to reflect this change. LISTS are now defined and declared separately from the eventhandlers. The corresponding invoke is EVENTHANDLER_DIRECT_INVOKE since I think EVENTHANDLER_LIST_INVOKE is too confusing (whereas DIRECT can reasonably imply direct disaptch-ish semantics).
• Now you must use EVENTHANDLER_LIST_DEFINE to create the reference to the list. EVENTHANDLER_LIST_DECLARE is only needed to declare the list pointer extern, so for callers of EVENTHANDLER_DIRECT_INVOKE that are not in the same compilation unit as the DEFINE.
• small change to make the eventhandler list name stored at the end of the struct, since the storage is always allocated at the end of the struct anyway.
• remove EHL_INITTED since it is no longer a relevant state (the lists are initialized either by the SYSINIT stage for eventhandler or when the first handler is registered).
• update EVENTHANDLER(9) to reflect the API addition, with the recommendation to use the EVENTHANDLER_LIST/DIRECT macros.

In D12814#267063, @markj wrote:

Seems ok to me. Do you plan to update this further in light of Ian's similar patch?

+1 to the man page update.

Plz

I'd rather not turn my (perhaps radical) parentheses opinions into a bikeshed. At the least I request not adding new ones that aren't needed, but if kib or someone else prefers they stay I won't get in the way.

The superfluous parentheses stuff is sort of nitpicky, but I do feel that at least in the case where the expansion is surrounded by commas then it should be dropped since comma operators / separators have the lowest possible precedence.

• whitespace

• Might as well use TAILQ_FOREACH_SAFE while we are here.

• use init_unrhdr instead of duplicating it

• Add comment explaining the conditional free_unr

• Fixup manpage.
• Move KASSERT.

For anyone wondering about the history here, see this thread from -net: https://lists.freebsd.org/pipermail/freebsd-net/2017-August/048621.html. TL;DR is this: https://lists.freebsd.org/pipermail/freebsd-net/2017-September/048826.html

In D12217#253705, @rozhuk.im-gmail.com wrote:

Now, no one needs an amdsmn.
If some module in future will use SMN then it will be easy to share this code into module.

That's true, but what's so offensive about having another module? Why do we need to wait for abundant reasons to have logical code separation as opposed to copy paste code sharing?

In D12217#253620, @avg wrote:

In D12217#253594, @rozhuk.im-gmail.com wrote:

I dont know where did you get info about Ryzen thermal interface, but for fam 15h 0x60 named "Miscellaneous Index" and I dont know about any renaming registers before.

Why amdsmn in new driver module?
(I dont think than any one want to do something with SMU or Miscellaneous registers)

Why do you think that there is any direction relation between SMU and Miscellaneous registers in family 15h and SMN in family 17h?
SMN is a completely new thing in family 17h (or so it seems), the only similarity between SMU and SMN is "SM" :-)
Family 17h is a big step from families 10h - 16h and even between those families there were incompatible changes in PCI register definitions (I know that for sure about registers that describe DRAM configuration).

Given the lack of documentation we can resort -- again! -- to using Linux commits by AMD employees as a guidance.
For example, https://patchwork.kernel.org/patch/9432511/

In D12217#253379, @cem wrote:

Yeah, that is an unfortunate problem of AMD's temperature scale. On the threadripper part, the offset is 27°C. I don't know how smart we want this thing to be. Is there a good way of determining specific model numbers? For what it's worth, I believe amdtemp had exactly the same problem on previous CPU generations.

My thought is that this is outside the scope of this revision since it's an existing problem in amdtemp. It's not totally clear to me what the best way to do it since the different parts have different offsets, but it's something someone should probably explore. If we can reliably map the offsets for every known part in every generation it would be a nice convenience feature.

Also missing the sys/modules/amdsmn directory, so doesn't work with buildkernel at the moment.

In D12188#252958, @np wrote:

In D12188#252955, @mjoras wrote:

When you commit can you be more explicit about why this fixes it with the extended media types? For people not familiar with if_lagg some more specifics in the message would be helpful.

I updated the review summary, which is going to be the commit message.

In D12188#252955, @mjoras wrote:

Also unless I'm mistaken it looks like IF_BRIDGE(4) has the same bug. Should we fix that as well?

Upon further inspection, it doesn't actually matter for bridge interfaces.

When you commit can you be more explicit about why this fixes it with the extended media types? For people not familiar with if_lagg some more specifics in the message would be helpful.

The integer handling looks correct. It is definitely better than the defined-but-bad behavior of truncating the non-fractional bits of sbintime_t on ILP32. Using unsigned values everywhere makes all the operations well-defined and reasonable.

So I will note that on my system hwpstate ends up opting to get these from acpi_perf:

kernel: hwpstate0: going to fetch info from acpi_perf
...
mjoras@icarium ~> sysctl dev.hwpstate.0.freq_settings
dev.hwpstate.0.freq_settings: 3400/3825 2800/2765 2200/1952

• braces