How about some attribution here to where this code is coming from!
Especially since there is a black history behind, I hope to never revive!

This kind of breaks the assumption of floating states, 'it will apply in any interface any direction'.
These states can be killed through src/dst options, or potentially you can do this in a daemon managing states in userland and killing states by id.

Why can't you move the interface 'all' to be a flag and just always track the interface the packet came in originally?

You can add me to these reviews @kp !
I have implemented originally all of this without thinking of generic way of reusability.
The intention is to have some metadata tagged on states and rules to easy perfom operation and/or reporting.
The model can be improved by having a better tagging model then just stash new fields in the structure, but porting first the patches and later on collapsing them can be a path forward, as long as the ABI is not impacted too much.

Not a blocker but:

• It would also be nice to have measure if the other side can keep up with the blast of state updates now?
• Even better, provide the same bucket mechanism on reception so it can be distributed on the various cores

Can you please measure the latency of syncing states with this change against previous latency?

Can you add some text to the manual pages for documenting the feature? Possibly linking to some example?

What needs to be considered here is the assumption of pfsync is that a state created in pf will be synched at shortest possible cycle to the cluster member.
By defering that assumption is relaxed so figuring out baselines of before and after this change would make this more easy to reason about.

Continue the great work you are doing on providing test cases for pf.

Looks good.

In D13715#295452, @kristof wrote:

In D13715#295449, @eri wrote:

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.

I'm not sure I see how this would create confusion. This merely presents more information about the packet, and where the netpfil hook being called from.

While i have not much time lately to spend on this, i still think this is the wrong way of doing things since it just creates confusion.
pf(4) has already knows about mbuf_tag(9) and uses it. I would strongly suggest using them until a proper _FWD hook comes to life and allows removing all the 'hacks' in pf(4) and possibly elsewhere.

While this is needed i do not agree that the modifications on the stack and packet filters should be so hackish.

Just curious, do you have any comparison/profiling data if this improves anything?

Good catch.

I would rather remove the pf_unload and pf_load to empty stubs and do their operation in pf_vnet_[un]init wiht DEFAULT_VNET wrapping than this.

It feels a lot like a hack.
Shouldn't the proper VNET accessor be called on creation and teardown?

In D9721#204349, @ae wrote:

In D9721#204330, @adrian wrote:

oh hm, one ipsec flow? so ok. So, can you also dump out the NIC statistics so we can see if the NIC is seeing all the traffic on one RX ring, or whether it's being load balanced between multiple RX rings.

Adrian, please, note that this netisr queue is for outbound traffic. It queues packets that are going to be encrypted. And these packets are ordered by SPI value, so for one flow there is no parallelism. Also there are no parallelism in the crypto(9) subsystem, so you wont get any benefits from many isr threads, they all will be blocked on the crypto thread.

Include documentation (man pages) though someone should give a review to those changes.

Update considering comments.

In D3133#198459, @ae wrote:

Also can you describe the cause of hang? From the patch it is not obviously to me.

Completed review tasks.

Update diff to include all remarks

Just some information to pkesley@ comment.

Just looking for an OK here this is not rocket science!

So i go and commit this in one week if no objections are raised.

Anyone has objections so this can go in?

Update to include context.

Busy with other things.
Updating this to include context.

Update patch to include context as well.

Updated as per comments.

Can't this be made part of a single API where the protocol family is passed so it can be reused and extended easily?
Imagine layer2 forwarding can use this as well.....

I understand and i still think this is _completely_ useless from what you have submitted.

This is not the way this should proceed.

You need to do more than this.
This change is not correct per se.

In D5017#108466, @ae wrote:

Can you link your patch with /head branch, so all context will be available?

In D5017#107730, @adrian wrote:

just make sure you test the RSS/PCBGROUPS options too. :)

-a

Any update on the following?

In D5017#106574, @gnn wrote:

I'm a bit confused by this proposal. No protocol I know of has a port field larger than 16 bits. How and where might this be applied?

I know this has been committed but please check my remarks.

The easiest way to do this is check if you have an inp and check that the proto is TCP from that and skip cksum update.
Otherwise you will end up with complex code as this which is easier to understand and maintain.
It certainly is not documented properly but this makes it even more.

The problem with IPSec is that this might leak traffic that otherwise would be used by ipsec.
Probably even looped traffic might leak with this scenarios from proxies....

This still breaks IPSEC scenarios at least.

See my replies!

Reduce the diff to only rule handling for now.

I would have put this in pf_refragment6 function call but lets put this in and see later on to improve it further.

Yeah i noticed this and am working on fixing it.
Mostly this is from the relaxed requirement to accept rules without existing interface name!

Did this behavior started after implementing the re fragmentation functinality?

While that is not an open available link but clearly the target is different.
That patch is about IPSec not impacting forwarding path when its not in use at all
while this patch is about avoiding a routing lookup when IPSec is in use more in relation with the commit in FreeBSD removing redundant routing table lookups in forwarding path.

Sure gnn@ i can write Obtained from eri at al.
What have you been smoking lately? Can you clarify where this obtained from should contain and you reasoning behind?

It looks good in general, though I do not like automatic conversion in such case.
Just put in the commit as part of release notes and people need to be fully aware of the change.
Whoever used these options i am certain had their reasons so they should be aware of the change.

I do not think this is the root cause for this, it just hides another issue.
I think that the result of RB_INSERT should be checked when a group/iface is created should be tested.
If that returns NULL the result should be null.

One thing not mentioned in this review is that the codel queue type can be used on any other scheduler like PRIQ, CBQ, HFSC, FAIRQ.
It is quite flexible at that.

This can go in.
I am working on restructuring all this to be a bit more modern and not so...hackish.

I do not see why the receiving of the ICMP_UNREACH is impacted by this?

Can you please put my copyright in the codel files since it was forgotten addition when i did the port?

Yea Andrey will do for IPv6.