LGTM, thanks!

update as mentioned

created new review instead of update

thanks a lot :)

In D31515#724915, @markj wrote:

Sorry for the delay. I have no objections to the change and the implementation looks fine. I am a little wary of committing it without some wider approval: could I ask you to post a short note to freebsd-net@ linking this diff and soliciting opinions? If there are no objections after a week or so I will commit this.

We have multiple reports that this causes throughput regressions when in use on 13-STABLE as opposed to 13.0-RELEASE where it is not present. We have had this commit reverted and speeds are back to normal for our OPNsense users. For more info see https://forum.opnsense.org/index.php?topic=26364.0

fqpie_callout_cleanup() should exhibit the same issue

In D33432#755856, @kp wrote:

In D33432#755851, @franco_opnsense.org wrote:

I was thinking the same at first but the locking introduced in https://cgit.freebsd.org/src/commit/sys/netpfil/ipfw/dn_aqm_pie.c?id=12be18c7d594 looks arbitrary and isn't anywhere else in those two files. It was added to "protect" the ref_count manipulation, but if you look at the other ref_count modification in that file these are also done without (obvious) locks.

Maybe these ref_count modifications should receive atomic updates without locks to avoid the locking overhead completely?

Perhaps, yes.

Although it looks like the ref_count is only read in unload_dn_aqm(), under the sched_mtx lock. That lock lives only in ip_dummynet.c, so I wonder if we shouldn't just move the updating of the reference count to dn_aqm_ref()/dn_aqm_unref() and protect it with the sched_mtx lock. That doesn't need vnet, so we don't have to worry about setting the context (because it's about a global setting, so using a vnet-ed lock is wrong anyway) and we actually clean the locking up a little.

I was thinking the same at first but the locking introduced in https://cgit.freebsd.org/src/commit/sys/netpfil/ipfw/dn_aqm_pie.c?id=12be18c7d594 looks arbitrary and isn't anywhere else in those two files. It was added to "protect" the ref_count manipulation, but if you look at the other ref_count modification in that file these are also done without (obvious) locks.

In D33432#755816, @kp wrote:

Do you have a description on how to trigger this panic?

In D8877#754326, @mjg wrote:

what's the fate of this patch?

Just as a comment: With all these ties to RSS defines I'm not sure where that leaves the RSS feature with regard to its multiple hardware/software use cases but there's no point in blocking this with no visible consumers. I'll make sure to give this a test once it hits main.

In D32585#738110, @glebius wrote:

In D32585#737897, @kbowling wrote:

In D32585#737015, @np wrote:

Note. This change requires PCBGROUP to be retired.

Have you circulated this proposal in the wider -net and -vendor community? I know of one downstream that uses this feature.

It looks like opnsense is trying to make use of pcbgroups/rss, might want to check with @franco_opnsense.org

I don't see much history here https://github.com/opnsense/src/commits/master/sys/netinet/in_pcbgroup.c
Anyway, adding Franco as reviewer.

Thanks for the find! Looks reasonable to bring in. I will try to get more test coverage from our users, though feedback was low on this particular hang ;(

Added motivation for checking for untagged priority to the filter program comments.

add comment about need to test for VID 0 presence

Not sure about omitting the match on a NAT rule, but doing it inside the log code was definitely wrong.

void REASON_SET by directly passing PFRES_MATCH

But to be fair both rules are matching accounting-wise unless we assume that only "pass" can account for "match".

There is an older discussion about it here https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=224961

• skip to end on vlanid != 0 and add comments

Correct. Here is the test:

• lease declaration skip fixes

Use EVL_VLID_MASK as suggested

OpenBSD added the single htons() while adding write filter, but nowhere else. I suspect the fragmentation check is mostly correct so this doesn't matter in the real world.

constify filters and avoid static length variables

Appreciate the reviews :) Unfortunately I'm not a committer so is someone willing to help out? Thanks in advance.

fixed partial length on tx

Another one refactored

That would be great, thanks!

The only other script is the DNS script and it looks like -u already does append the sender address to that script's data.

now avoids raw access to ntopbuf from scripts call

Merged into https://reviews.freebsd.org/D31501

Much nicer, thanks! Added Stephan as reviewer who originally worked on this to give it a go.

On second thought: maybe I'm mixing up hardware. I'm not sure, but in either case 12.x does seem not exhibit this issue judging from the complete lack of user reports.

@kbowling for us https://cgit.freebsd.org/src/commit/?id=0aa7d3ff9ea resolved the issue back then and has not come back. Is this meant to replace it or to be applied on top? I can test it on the specific hardware, but unsure if this will be inconclusive for the mentioned reason

Thanks for the update :)

@skozlov feel free to take maintainership of net/intel-em-kmod and thanks for working on these modules! :)

"if:0" and "(if:0)" have separate implementations. the one for "if:0" is missing, see ifa_lookup() in sbin/pfctl/pfctl_parser.c

Looks good, thanks!

We all invest time so thank you for the offer. Yes, I am OK with that.

I do not wish to acknowledge the advertising in the commit message.

I'm sorry, I'm playing by the rules that FreeBSD imposes on its external contributors.

There's a PR here https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229904

Any news on this? :)

Seeing this after migration...

this one is stable, thank you @ale