Bug 240787 - netgraph/ng_bridge: Replace NG_BRIDGE_MAX_LINKS with auto-incrementing (Unlimited) links
Summary: netgraph/ng_bridge: Replace NG_BRIDGE_MAX_LINKS with auto-incrementing (Unlim...
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.0-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: Gleb Smirnoff
URL: https://reviews.freebsd.org/D21803
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-24 11:14 UTC by lutz
Modified: 2019-10-11 09:19 UTC (History)
6 users (show)

See Also:
koobs: mfc-stable11-
koobs: mfc-stable12-


Attachments
remove fixed limit for links on netgraph/ng_bridge (10.29 KB, patch)
2019-09-24 11:14 UTC, lutz
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description lutz 2019-09-24 11:14:16 UTC
Created attachment 207757 [details]
remove fixed limit for links on netgraph/ng_bridge

The netgraph node ng_bridge has a compile time limit for the number of links, it can handle.

This patch removes this limit by allocating more links when needed (assuming sequential assignment).

Please note, that this patch is preliminary. It's only done to allow an incremental review of the development.

A large number of links require further changes to the code in order to handle all cases efficient. This includes:
 + replacement of the central "priv->link" table by hook private data
 + replacement of foreach loops by a linked list over looped hooks

In consequence the current requirement to use a compact, sequential assignment of "link%d" hooks can be replaced by arbitrary naming conventions.

How should I provide the additional patches? A large patch (overwriting the previous one) or by a series of patches?
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-24 11:28:31 UTC
Could this be made into a run-time tunable (sysctl), at least as a simplification while auto-incrementing design is worked on/reviewed ?
Comment 2 lutz 2019-09-24 12:13:59 UTC
The idea is to replace the whole array by hook private storage.
This will remove the need for any limit control.

Should I provide a summary patch as replacement?
Comment 3 Aleksandr Fedorov 2019-09-26 08:36:43 UTC
What do you think about submitting this patch to the phabricator?

https://reviews.freebsd.org/
https://wiki.freebsd.org/Phabricator
Comment 4 lutz 2019-09-26 09:02:53 UTC
Thank you for pointing me to phabricator.
I'll try to find out how it works.
Comment 5 lutz 2019-09-26 15:52:28 UTC
Moved to https://reviews.freebsd.org/D21803
Comment 6 lutz 2019-09-27 11:54:50 UTC
manpages review is "green"

What are the next steps @Phabricator?
Comment 7 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-01 02:21:03 UTC
Comment on attachment 207757 [details]
remove fixed limit for links on netgraph/ng_bridge

^Triage: Obsoleted by https://reviews.freebsd.org/D21803
Comment 8 commit-hook freebsd_committer 2019-10-03 02:33:39 UTC
A commit references this bug:

Author: glebius
Date: Thu Oct  3 02:32:56 UTC 2019
New revision: 353026
URL: https://svnweb.freebsd.org/changeset/base/353026

Log:
  - Remove the compile time limit for number of links a ng_bridge node
    can handle.  Instead using an array on node private data, use per-hook
    private data.
  - Use NG_NODE_FOREACH_HOOK() to traverse through hooks instead of array.

  PR:		240787
  Submitted by:	Lutz Donnerhacke <lutz donnerhacke.de>
  Differential Revision:	  https://reviews.freebsd.org/D21803

Changes:
  head/share/man/man4/ng_bridge.4
  head/sys/netgraph/ng_bridge.c
  head/sys/netgraph/ng_bridge.h
Comment 9 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-03 12:07:11 UTC
Is this a merge to stable/* or releng/12.1 candidate?
Comment 10 lutz 2019-10-03 12:29:23 UTC
I would welcome a merge to STABLE, because that's the production environment.
Comment 11 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-04 02:25:46 UTC
^Triage: 12.1-R candidate
Comment 12 Mark Johnston freebsd_committer 2019-10-08 14:44:37 UTC
(In reply to Kubilay Kocak from comment #11)
Why is this a 12.1 blocker?  The change is a welcome improvement but does not fix a regression.  Rather it removes an existing limitation in ng_bridge(4).  IMHO it is too late to include this in the release.
Comment 13 Gleb Smirnoff freebsd_committer 2019-10-08 15:27:06 UTC
I totally agree with Mark.
Comment 14 Ed Maste freebsd_committer 2019-10-08 16:14:23 UTC
Too late for 12.1, but merging to stable/12 before 12.2 is probably feasible
Comment 15 Julian Elischer freebsd_committer 2019-10-08 21:11:47 UTC
it changes the API (and the cookie that identifies the API revision) so old binanries will not work. This sort of disqualifies it from 12.anything.. remember that we keep API along a branch lifetime.  For now we should just increase the default number of links in 12.
Comment 16 Julian Elischer freebsd_committer 2019-10-09 02:01:56 UTC
though one could implement the old cookie too and translate to some set of defaults in the new scheme.
Comment 17 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-11 08:56:36 UTC
^Triage: 

- Remove block on 12.1-R issue
- Decline stable/* due to ABI change (comment 15)

If anyone wants to change this, just re-open and set mfc-stable* back to ? accordingly with a comment
Comment 18 lutz 2019-10-11 09:17:04 UTC
https://reviews.freebsd.org/D21961 solves the ABI change issue.
It's accepted but not committed.
Comment 19 Kubilay Kocak freebsd_committer freebsd_triage 2019-10-11 09:19:42 UTC
(In reply to lutz from comment #18)

Please ask the committer who intends to commit the change to reference this PR in the commit log message so it and the subsequent MFC can be tracked here