ng_ether: implement 'attach'

[kp]

Summary

ng_ether allows nodes to be detached from their interfaces, but provided
no way to re-attach.
Implement the 'etherattach' command for this.

Obtained from: pfSense
Sponsored by: Rubicon Communications, LLC ("Netgate")

[donner]

This proposed message is specific to the ng_ether node. There is no need to clobber the global message space with such a specialized message.

[kp]

In D32550#734616, @donner wrote:

This proposed message is specific to the ng_ether node. There is no need to clobber the global message space with such a specialized message.

I may not know enough about netgraph, but I'm not sure how we could put it in the ether namespace. It can't be directed to the ng_ether node, because that doesn't exist at the time we try to (re-)attach.

[donner]

In D32550#734619, @kp wrote:

I may not know enough about netgraph, but I'm not sure how we could put it in the ether namespace. It can't be directed to the ng_ether node, because that doesn't exist at the time we try to (re-)attach.

Ah, I see. The node is destoyed by receiving the "detach" message. The assumption behind is, that the corresponding real interface is going to be destroyed, too.

May you describe your use case, where the interface persists, while the netgraph node gets destroyed?

[kp]

In D32550#734620, @donner wrote:

In D32550#734619, @kp wrote:

I may not know enough about netgraph, but I'm not sure how we could put it in the ether namespace. It can't be directed to the ng_ether node, because that doesn't exist at the time we try to (re-)attach.

Ah, I see. The node is destoyed by receiving the "detach" message. The assumption behind is, that the corresponding real interface is going to be destroyed, too.

May you describe your use case, where the interface persists, while the netgraph node gets destroyed?

In part it's to provide symmetry with the existing detach call. The original (Netgate internal) commit message also mentions a desire to avoid netgraph overhead ("This should prevent the netgraph queue to slow down network performance on fast links."), although I've not yet been able to measure any such impact myself. pfSense will actively detach interfaces unless/until it wants to use netgraph on them.

[lwhsu]

usr.sbin/ngctl/etherattach.c

18

I think this line also needs to be updated.

[lwhsu]

Hum, there are more "WHISTLE" in the copyright header of usr.sbin/ngctl/etherattach.c.

I usually copying from FreeBSD Project's entry in /COPYRIGHT which uses "AUTHOR AND CONTRIBUTORS".

[kp]

Fix copyright header (thanks lwhsu).

[donner]

In D32550#734666, @kp wrote:

In part it's to provide symmetry with the existing detach call. The original (Netgate internal) commit message also mentions a desire to avoid netgraph overhead ("This should prevent the netgraph queue to slow down network performance on fast links."), although I've not yet been able to measure any such impact myself. pfSense will actively detach interfaces unless/until it wants to use netgraph on them.

The netgraph node is attached to the interface, during loading the ng_ether module. If you deliberately choose to disassociate the node (hence destroying it), then this is similar to unloading the module. In principle, we are talking about a boot process action.

So if you detach/delete the node for an interfaces, you can either "hotplug" remove and reinsert it. This will cause a generation of the new ng_ether node. Or simply reboot.

OTOH, if you really think about a method to attach a ng_ether node to an existing interface again, I'd suggest to handle this in a more node-local way:

• Create a new ng_ether node using ngctl (which is currently not implemented)
• The node creation is only parametrized by the hook names (local and peer)

Consequently the idea would be to implement ng_ether_constructor by taking the peer node name as the name of the interface to attached.

> mkpeer . ng_ether ixl12 lower    # create the ng_ether node for the ixl12 interface
> rmhook ixl12                     # node can live standalone
> show ixl12:                      # node was automatically named correctly
  Name: ixl12   Type: ether    ID: 15d8     Num hooks: 0

Would this please your needs?

[donner]

A complete different approach would be to call ether_reassign from if_ethersubr.c

#ifdef VIMAGE
void
ether_reassign(struct ifnet *ifp, struct vnet *new_vnet, char *unused __unused)
{        

        if (ifp->if_l2com != NULL) {
                KASSERT(ng_ether_detach_p != NULL,
                    ("ng_ether_detach_p is NULL"));
                (*ng_ether_detach_p)(ifp);
        }
 
        if (ng_ether_attach_p != NULL) {
                CURVNET_SET_QUIET(new_vnet);
                (*ng_ether_attach_p)(ifp);
                CURVNET_RESTORE();
        }
}
#endif

[donner]

For the performance aspect, the following code path is involved:

        /* Handle ng_ether(4) processing, if any */
        if (ifp->if_l2com != NULL) {
                KASSERT(ng_ether_output_p != NULL,
                    ("ng_ether_output_p is NULL"));
                if ((error = (*ng_ether_output_p)(ifp, &m)) != 0) {
bad:                    if (m != NULL)
                                m_freem(m);
                        return (error);
                }
                if (m == NULL)
                        return (0);
        }

/*
 * Handle a packet that is going out on an interface.
 * The Ethernet header is already attached to the mbuf.
 */
static int
ng_ether_output(struct ifnet *ifp, struct mbuf **mp)
{
        const node_p node = IFP2NG(ifp);
        const priv_p priv = NG_NODE_PRIVATE(node);
        int error = 0;

        /* If "upper" hook not connected, let packet continue */
        if (priv->upper == NULL)
                return (0);
[...]

If you have a disconnected node on the interface, you add a subroutine call and a comparison ... compared to the pure interface.
Do your really have such a hard performance requirement, that those calls do matter?
Did you also remove the carp and bridge hooks?

[kp]

In D32550#734831, @donner wrote:

If you have a disconnected node on the interface, you add a subroutine call and a comparison ... compared to the pure interface.

Yeah, I know. I'm not sure what the historic commit message refers to. I've been unable to demonstrate a performance impact in my own tests (although testing can only prove the presence of bugs, not their absence, of course).

Do your really have such a hard performance requirement, that those calls do matter?

Yes and no. We care about performance, and many small effects do add up, but not everything affects it enough to matter.

Did you also remove the carp and bridge hooks?

No.

As I said: this is part of an effort to reduce the diff between FreeBSD and pfSense. I have two arguments for this patch (i.e. we can currently detach, but not re-attach, and the historic commit message suggesting a performance impact). I recognise that neither one is extremely strong, but taken together it seemed worth it to at least discuss it.

I'll see if I can rework the patch along the lines you suggested, because I do agree that it's kind of icky to have ng_ether specific commands in the ng core code.

[glebius]

I agree that the ng_ether design with automatic creation of a node proved to be problematic. Ideally it should be explicitly attached and detached. The suggested patch will do that.

However, more ideally we need a generic API to reconfigure interfaces input routines to different pipes: netgraph, lagg, bridge, some future stuff. Maybe documentation should note that current way of configuring ng_ether may change in future.

[donner]

In D32550#735434, @glebius wrote:

I agree that the ng_ether design with automatic creation of a node proved to be problematic. Ideally it should be explicitly attached and detached. The suggested patch will do that.

Then I'd prefer the "in-node" code path over a generic message.
@kp should I try to implement such an approach?
i.e. ngctl mkpeer . ng_ether lagg1 lower

[glebius]

I like Lutz suggestion. Of course better not to create a generic message for a specific node. Maybe add new hook to ng_ether, to avoid traffic flowing immediately into ng_socket instead of the network stack after attach? A new hook can be "upper write only", that will allow to inject packets into Ethernet, but not intercept anything, which is a useful feature itself.

[afedorov]

I am also against the use of generic messages.

Maybe add a special hook called "attach" that will reattach the interface to ng_ether?
Then, as Lutz suggested: ngctl mkpeer. ng_ether lagg1 attach

[kp]

In D32550#735557, @donner wrote:

In D32550#735434, @glebius wrote:

I agree that the ng_ether design with automatic creation of a node proved to be problematic. Ideally it should be explicitly attached and detached. The suggested patch will do that.

Then I'd prefer the "in-node" code path over a generic message.
@kp should I try to implement such an approach?
i.e. ngctl mkpeer . ng_ether lagg1 lower

I'd be delighted if you could.

[kp]

Consensus is that this is the wrong approach.