In D44488#1031012, @markj wrote:

but with this change we are implicitly rewriting the source port. What explicit mechanism is the man page alluding to here?

That'd be things like rdr on $ext_if proto tcp from any to any port 80 -> 127.0.0.1 port 8080, where we redirect incoming connections on port 80 to localhost port 8080.
When we're doing explicit rewrites it's always going to be the destination port, of course.

I'm not sure I'd have bothered to make the nesting level configurable, but now that you've done the work it'd be silly to not use it.

The referenced commit (2b9600b4497b) seems to be relevant for the detach flow, but it doesn't guarantee there will an if_bpf pointer when a struct ifnet is created.
However, ether_ifattach() does an unconditional bpfattach(), so I think this is still correct (given that mlx* are Ethernet devices).

In D44774#1028978, @lwhsu wrote:

My personal suggestion: it's worth to mention it in carp(4), and perhaps even adding a link of vrrp(4)

Last call for objections. I intend to commit tomorrow.

In D44774#1028213, @bz wrote:

In D44774#1027883, @kp wrote:

In D44774#1027778, @bz wrote:

(2) We are adding to but not fixing the problem of the conflicting version number; it's easy to say I can add v3 but ... see (1).

As in carpv3? That seems entirely unlikely to ever happen, given that carp's entire reason for existing is now moot.

No, as in carp getting an official version number.

I'm not sure I follow. Do you mean carp might be officially designated vrrp version 9 (let's say)? That seems both very unlikely, and something that wouldn't actually be a problem to deal with. We already distinguish vrrpv3 from carp by the version number, so looking for version 9 would, if anything, make things easier.

review remarks

In D44774#1027778, @bz wrote:

I'll reply here rather than the email--one well two major concerns (and I didn't mean legal) with the "can of worms" are:
(1) You are adding VRRP version 3... someone will come next and ask "and what about VRRP version 2"?

Right now the answer is "We don't have v2, and we have no plans to support v2 either.".
If someone does ever want to add VRRPv2 support they get to deal with the fallout. I'm not inclined to copy/paste chunks of code on the off chance that maybe it'll be slightly helpful in the future.
We're not making choices that'll make that impossible to do in the future.

In D45039#1027522, @glebius wrote:

Is this whole stack of reviews a temporary reviews to get easier review for pfsense and network? I think better just squash all my submissions on top of your work and push that to FreeBSD/main as one change.

In D44774#1026860, @kp wrote:

Split out Gleb's commits again.

It makes the individual changes easier to understand.

Split out Gleb's commits again.

Document that we cannot switch between carp and vrrp

• remove stale comment
• init sc_addr in the initializer

Add glebius' changes from https://github.com/freebsd/freebsd-src/compare/main...glebius:FreeBSD:carp-vrrp

In D44774#1024964, @glebius wrote:

If it is possible to set VRRP values while interface is in CARP mode and vice versa, then it is already a bug and de-unionizing just hides it.

In D44774#1024470, @glebius wrote:

The switching code should just use a local variable to convert between prio and adnskew.

We can't convert between them. They're not different representations of the same concept, they just represent different things.

In D44774#1024364, @glebius wrote:

In D44774#1023611, @kp wrote:

Put carp/vrrp3 specific variables in their own structs.

This definitely is better, but I still can't figure out what prevents to unionize it? Do we have a code that switches certain address operation between CARP and VRRP?

In D44774#1020944, @kp wrote:

In D44774#1020899, @bz wrote:

I will simply express that this will not only open a can of worms by mixing both but the original reasons not to include VRRPv2/3 and hence the "existence" of CARP is also ignored.

I'm assuming you're referring to the supposed patent issues?

There are multiple other open source VRRP implementations, e.g. https://github.com/FDio/vpp/tree/master/src/plugins/vrrp and https://www.keepalived.org
Also, the relevant patents have expired by now (by which I mean, in 2017 and 2012, so 7 and 12 years ago): https://en.wikipedia.org/wiki/Virtual_Router_Redundancy_Protocol#cite_note-6

Put carp/vrrp3 specific variables in their own structs.

• review remarks

In D44774#1020899, @bz wrote:

I will simply express that this will not only open a can of worms by mixing both but the original reasons not to include VRRPv2/3 and hence the "existence" of CARP is also ignored.

I'm assuming you're referring to the supposed patent issues?

Allow MIB SDT's to be disabled.

In D43504#1017242, @gallatin wrote:

In the meantime, could you have a compile option to avoid these probes? Maybe re-do your new macros to use IP_SDT_PROBE" which can be defined to a noop if "NO_EXTRA_IP_SDT_PROBES" or some better named kernel config option is present?

In D43504#1016001, @gallatin wrote:

I lost my benchmark box yesterday. It will take me a few days to wrangle another one into shape for testing. I'm sorry this is taking so long. If you want to go ahead and push this, I'd understand..

This also really needs a test case.

In D43504#1014598, @gallatin wrote:

I think what I will test is:

1. SDT probes compiled out entirely
2. This (D43504) patch with our current SDT mechanism
3. D43504 + D44483 together

Does that sound appropriate?

@olivier : Do you have the time to do the same sort of test on your low-end pps routing setup?

In D44476#1014459, @glebius wrote:

In D44476#1014446, @kp wrote:

When we build without VIMAGE VNET_SYSUNINIT translates to SYSUNINIT, so this patch means we leak V_icmp_rates[i].cr_rate on shutdown.
That's not exactly a critical problem, but this is technically wrong.

I don't agree with that. We don't deallocate memory on shutdown in general case. We do not have a matching SYSUNINIT for every SYSINIT that mallocs. Keeping a function to deallocate memory on shutdown is the actual waste of memory - it grows kernel text, which is wired.

When we build without VIMAGE VNET_SYSUNINIT translates to SYSUNINIT, so this patch means we leak V_icmp_rates[i].cr_rate on shutdown.

In D43504#1013955, @glebius wrote:

To put it lightly, I'd really like to see this patch performance tested.