In D40348#918719, @markj wrote:

In D40348#918710, @melifaro wrote:

I just experienced a similar scenario and complained to melifaro that netlink stopped working in that scenario. The ideal situation would be that netlink would just malloc memory, or use its own dedicated zone rather than mbufs.

Netlink by itself is opaque to underlying memory type (and uses plain malloc for linux app buffers / cases when the requested netlink message size is > 2k). I'd also prefer not to use mbufs at all, but that's the current way of interacting with socket buffers. To switch from that, one needs to implement routines like soreceive_generic, which is gigantic 500-line function. I'm looking into the alternative approaches, but for now let's try to get maximum from the current implementation

It is possible to provide an alternate allocator for mbufs. m_free() will invoke a custom destructor for M_EXT mbufs, so you can bypass the normal UMA zones used for network packets.

Ack. Thank you for the hint, I've created D40356, implementing this approach. It looks better than the current fix, so I'd prefer to land the new diff instead.

In D40348#918709, @adrian wrote:

I think having a separate dedicated allocator for netlink messages is the better call. Don't make your control plane reliant on your data plane's memory management.

I just experienced a similar scenario and complained to melifaro that netlink stopped working in that scenario. The ideal situation would be that netlink would just malloc memory, or use its own dedicated zone rather than mbufs.

Netlink by itself is opaque to underlying memory type (and uses plain malloc for linux app buffers / cases when the requested netlink message size is > 2k). I'd also prefer not to use mbufs at all, but that's the current way of interacting with socket buffers. To switch from that, one needs to implement routines like soreceive_generic, which is gigantic 500-line function. I'm looking into the alternative approaches, but for now let's try to get maximum from the current implementation

No concerns from my side.

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

I've raised D40332 to address lack of notifications to carp and the rest of the system.

Wow, that's nice!
LGTM, though I'd suggest trying to add structured output from day one. I managed to convert ndp(8) ( D35677 ), so if you could consider looking into that option, that would be awesome!

In D40317#917957, @kp wrote:

There's a rather annoying bug left in this. If we add a carp address on an interface that's down (i.e. IFF_UP is not set) the interface will come up during this, but remain in SUPPRESS state.

That's because the SIOCSIFADDR ioctl is passed to ether_ioctl(), which sets IFF_UP in ifp->if_flags directly, without any event handling. Ordinarily we'd call if_up(), which gives carp a chance to react to the interface coming up. That's not the case here, so it stays in SUPPRESS.

Initial attempts to fix this, by replacing ifp->if_flags |= IFF_UP in ether_ioctl() with if_up(ifp) resulted in ifconfig: ioctl (SIOCAIFADDR): File exists when trying to set an address on the interface. There's clearly an order of operations problem around this, but I don't fully understand it.

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

I'm afraid I won't be able to review this in the foreseeable future. I think I completely forgot pre-nexthop routing logic.

Ping :-)
I'd love to get rid of if_clone_advanced() before 14 and cxgbe is the only remaining user :-)

I would like to try to have another iteration of the discussion :-)

Also: I guess, there should be another change that enables use of this hook in pf?

LGTM!
I'd be nice to add the new hook to pfil(9) for consistency.

Update to latest HEAD & address comments

Not insisting, but maybe it's a good time to look at D38888 once again? :-)

Address comments.

In D40183#915542, @melifaro wrote:

In D40183#915476, @jhb wrote:

I feel like the worst offenders are really when you have to #ifdef for both for LINT-NOIP. Another tact we might consider is just axeing LINT-NOIP. I can see some value in LINT-NOINET so an INET6-only kernel remains clean, but I see less value in LINT-NOIP and I feel like more of the cost is for that config.

Typically, yes, it's the worst. However, unused functions also play a notable role. Re LINT-NOIP - I ran into some folks doing builds for embedded systems & complaining when it's broken.

I think rather than using evil greps in the build glue, I'd rather just add explicit lines to LINT-NO* that adjust the warnings FWIW if we want to consider doing this.

Thank you for the feedback!
There are tons of these greps in the kern.pre.mk so I assumed that's known acceptable evil :-) I don't have any opinion about the implementation.
I started with the explicit lines in LINT*, actually, but ran into a problem with extending CONF_CFLAGS. I'll retry tomorrow and publish the updated diff if I get it working.

The problem with the explicit lines in LINT that I see is that the actual folks who use noinet(6) combinations don’t use LINT configs. So, by doing changes on the LINT level we’ll remove useless reporting on the ci side. The actual users will still have to change their configs..
Thoughts?

In D40183#915476, @jhb wrote:

I feel like the worst offenders are really when you have to #ifdef for both for LINT-NOIP. Another tact we might consider is just axeing LINT-NOIP. I can see some value in LINT-NOINET so an INET6-only kernel remains clean, but I see less value in LINT-NOIP and I feel like more of the cost is for that config.

Typically, yes, it's the worst. However, unused functions also play a notable role. Re LINT-NOIP - I ran into some folks doing builds for embedded systems & complaining when it's broken.

I think rather than using evil greps in the build glue, I'd rather just add explicit lines to LINT-NO* that adjust the warnings FWIW if we want to consider doing this.

Thank you for the feedback!
There are tons of these greps in the kern.pre.mk so I assumed that's known acceptable evil :-) I don't have any opinion about the implementation.
I started with the explicit lines in LINT*, actually, but ran into a problem with extending CONF_CFLAGS. I'll retry tomorrow and publish the updated diff if I get it working.

In D40207#915290, @otis wrote:

In D40207#915289, @melifaro wrote:

In D40207#915288, @otis wrote:

It's not OK, because the build was fixed, although:

root@b14:~ # ifconfig vtnet0 inet 100.64.0.14/24
Bus error (core dumped)

I'll investigate whether this is only my problem (although this box is pretty vanilla -CURRENT).

Hm. is this amd64?

Yes, amd64, clean build from today's afternoon (CEST).

Any chance you're in IRC/slack/any other IM and can debug with me?
If not, I'll disable Netlink switch and we'll figure it out offline.
For me, ifconfig works and passes all tests.

In D40207#915288, @otis wrote:

It's not OK, because the build was fixed, although:

root@b14:~ # ifconfig vtnet0 inet 100.64.0.14/24
Bus error (core dumped)

I'll investigate whether this is only my problem (although this box is pretty vanilla -CURRENT).

Whoops. How did I miss that?

Good example happening right now - ac6dd012590e01f6a5fef490d52ffb4a6ca97798 and c98146ae229cc60834bfbecc229f2f7725e458d0 wouldn't be required with this change in place.

In D40183#914803, @imp wrote:

I'm not sure I like this... I'm not a fan of disabling warnings since it could hide other issues that creep in...

I don't like disabling warning either - they help maintain the code at better shape. Here we don't disable warnings for GENERIC (and any other config which has INET and INET6 compiled) so we should get all benefits provided by the current set of warnings.
Not failing on unused-variable, unused-but-set-variable and unused-function happens only for the NOINET[6]/NOIP code (otherwise it'll be caught in GENERIC). I don't see any issues that could come from having these 3 flags enabled. Maybe I'm missing something - so happy to discuss the concerning examples & address them.

Cleanup.