|Summary:||netgraph/ng_bridge: Replace NG_BRIDGE_MAX_LINKS with auto-incrementing (Unlimited) links|
|Component:||kern||Assignee:||Bjoern A. Zeeb <bz>|
|Severity:||Affects Some People||CC:||aleksandr.fedorov, bz, emaste, glebius, julian, markj, net|
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 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 6 lutz 2019-09-27 11:54:50 UTC
manpages review is "green" What are the next steps @Phabricator?
Comment 7 Kubilay Kocak 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 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 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 2019-10-04 02:25:46 UTC
^Triage: 12.1-R candidate
Comment 12 Mark Johnston 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 2019-10-08 15:27:06 UTC
I totally agree with Mark.
Comment 14 Ed Maste 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 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 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 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 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
Comment 20 commit-hook 2020-01-05 19:15:00 UTC
A commit references this bug: Author: bz Date: Sun Jan 5 19:14:17 UTC 2020 New revision: 356386 URL: https://svnweb.freebsd.org/changeset/base/356386 Log: netgraph/ng_bridge: Reestablish old ABI In order to be able to merge r353026 bring back support for the old cookie API for a transition period in 12.x releases (and possibly 13) before the old API can be removed again entirely. Suggested by: julian Submitted by: Lutz Donnerhacke (lutz donnerhacke.de) PR: 240787 Reviewed by: julian MFC after: 2 weeks X-MFC with: r353026 Differential Revision: https://reviews.freebsd.org/D21961 Changes: head/sys/netgraph/ng_bridge.c head/sys/netgraph/ng_bridge.h
Comment 21 Kubilay Kocak 2020-01-25 03:31:06 UTC
^Triage: Over to bz@ for closing pending merge of base r356386
Comment 22 commit-hook 2020-01-28 17:39:08 UTC
A commit references this bug: Author: bz Date: Tue Jan 28 17:39:03 UTC 2020 New revision: 357206 URL: https://svnweb.freebsd.org/changeset/base/357206 Log: MFC r353026,353030,354244 (glebius), r356386: 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. Reestablish old ABI. PR: 240787 Submitted by: Lutz Donnerhacke (lutz donnerhacke.de) Changes: _U stable/12/ stable/12/share/man/man4/ng_bridge.4 stable/12/sys/netgraph/ng_bridge.c stable/12/sys/netgraph/ng_bridge.h
Comment 23 Bjoern A. Zeeb 2020-01-28 17:40:51 UTC
Merged to FreeBSD 12 as discussed in this thread with the follow-up work from Lutz to re-establish the old API.