Bug 278130 - [netgraph][patch] ng_bridge hooks should auto assign number for [up]link
Summary: [netgraph][patch] ng_bridge hooks should auto assign number for [up]link
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 14.0-STABLE
Hardware: Any Any
: --- Affects Only Me
Assignee: Gleb Smirnoff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-04-02 14:37 UTC by David Marker
Modified: 2024-04-08 17:49 UTC (History)
3 users (show)

See Also:


Attachments
patches and example ngctl for testing (28.00 KB, application/x-tar)
2024-04-02 14:37 UTC, David Marker
no flags Details
updated patch for CURRENT (6.91 KB, patch)
2024-04-04 14:25 UTC, David Marker
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Marker 2024-04-02 14:37:01 UTC
Created attachment 249661 [details]
patches and example ngctl for testing

The removal of NG_BRIDGE_MAX_LINKS in review D21803 has made it possible to have way more than 32 connections to an ng_bridge. But there isn't any (easy) way for userland to track available link names and requires listing all existing [up]links or to just guess a number until one succeeds.

There appears to never be a need to know the link number of the hook on the ng_bridge. When an ng_bridge shuts down it disconnects all hooks. If you know the path of your ng_eiface, ng_socket, ng_ether, etc. you have a way to
`rmhook` without the link path. That is you can remove via either:
        rmhook <ng_bridge path> [up]linkX
or with
        rmhook <ng_eiface path> ether
Everything else that connects to ng_bridge has a similar option.

This change, however, is about removing the need to specify an [up]link number
when creating a connection to the ng_bridge. Using ng_eiface again it means
you can use:
        connect <ng_eiface path> <ng_bridge path> ether link
The ng_bridge will auto assign the lowest available [up]link number that is
valid.

The number space for [up]link is as large as an int, limited by the fact that
all netgraph nodes use an int to track the number of connected hooks they have, nd_numhooks in struct ng_node. This turns out to be a convenient fact when introducing unr(9) as I can just initialize to INT_MAX. But it also means you could just take advantage of that and create a numbering scheme in user space. For example:
        0,1 always reserved for ng_ether (which requires 2 hooks)
        16 - N reserved for bhyve
        N - INT_MAX reserved for jails
The only issue is that 3rd party scripts would have to agree (e.g. ezjail,
vm-bhyve, etc.) Each script would have to scan and when it finds an available
[up]link number hope its still there when it attempts the `connect`.

"uplink" was introduced with review D23963. I have preserved the requirement you still can not have "uplink0". That will be automatically handled if you connect and request "uplink", it will give you "uplink1" or higher depending on availability but never "uplink0".

Because [up]link number is not required it also becomes possible to inspect
a netgraph node type and based on that determine the `peerhook` argument to connect to an ng_bridge. I added a change to bhyve to do exactly that.

This allows for a simplification when using netgraph with bhyve, which became
possible with review D24620.
  Instead of:
    -t netgraph,socket=vm0,path=vmbridge:,hook=vm0link,peerhook=link0
  You can shorten to just:
    -t netgraph,path=vmbridge:

  Instead of:
    -t netgraph,socket=vm1,path=igb0:,hook=vm1link,peerhook=lower
  You can shorten to just:
    -t netgraph,path=igb0:

I'm just removing the need for `peerhook`. No need to specify `socket` or `hook`
as the code exists today. Without this you could start just passing "link" for peerhook when using an ng_bridge. It makes the bhyve change optional, and I'm ok if its removed, I mostly care about not having to deal with numbering links for my jails that use ng_eiface.

This change is not 100% backward compatible though. The unr(9) is shared by
`link` and `uplink`. The current code will allow `link1` and `uplink1` and this
change will not. The total number of [up]links is unchanged. But bear in mind uplink is for a physical interface connected to an ng_bridge, and the expectation is that you have only one of these, so it would be wasteful to have a separate unr(9) for just "uplink". But other than that it does still allow users to specify [up]link number.

The tarball contains patches for CURRENT and stable/14. stable/13 still uses the
the array of 32 hooks with NG_BRIDGE_MAX_LINKS. It also contains some simple tests to try with ngctl(8).
Comment 1 Aleksandr Fedorov freebsd_committer freebsd_triage 2024-04-02 18:46:34 UTC
Hi David! It would be nice to post a patch for ng_bridge(4) on the https://reviews.freebsd.org/. You can do this via the web interface according to the instructions: https://wiki.freebsd.org/Phabricator (Create a Revision via Web Interface)
 

A quick inspection of the patch showed that I have something to recommend.
Comment 2 David Marker 2024-04-03 14:26:34 UTC
Added review D44615 as requested.
Comment 3 Gleb Smirnoff freebsd_committer freebsd_triage 2024-04-03 17:18:15 UTC
Thanks for submission! Already working on comments in the review.
Comment 4 David Marker 2024-04-03 21:59:41 UTC
attached patches out of date. review has current.
Comment 5 David Marker 2024-04-04 14:25:55 UTC
Created attachment 249705 [details]
updated patch for CURRENT

updating patch for CURRENT to match the review
Comment 6 commit-hook freebsd_committer freebsd_triage 2024-04-08 17:49:10 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=86a6393a7d6766875a9e03daa0273a2e55faacdd

commit 86a6393a7d6766875a9e03daa0273a2e55faacdd
Author:     David Marker <dave@freedave.net>
AuthorDate: 2024-04-08 17:48:22 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2024-04-08 17:48:22 +0000

    ng_bridge: allow to automatically assign numbers to new hooks

    This will allow a userland machinery that orchestrates a bridge (e.g. a
    jail or vm manager) to not double the number allocation logic.  See bug
    278130 for longer description and examples.

    Reviewed by:            glebius, afedorov
    Differential Revision:  https://reviews.freebsd.org/D44615
    PR:                     278130

 share/man/man4/ng_bridge.4 |  13 +++++-
 sys/netgraph/ng_bridge.c   | 104 +++++++++++++++++++++++++++++++++++----------
 2 files changed, 93 insertions(+), 24 deletions(-)