From nobody Mon Jan 31 19:33:08 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 64CA2198DBAF; Mon, 31 Jan 2022 19:33:23 +0000 (UTC) (envelope-from glebius@freebsd.org) Received: from cell.glebi.us (glebi.us [162.251.186.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "cell.glebi.us", Issuer "cell.glebi.us" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JndX23Xsdz3rCc; Mon, 31 Jan 2022 19:33:22 +0000 (UTC) (envelope-from glebius@freebsd.org) Received: from cell.glebi.us (localhost [127.0.0.1]) by cell.glebi.us (8.16.1/8.16.1) with ESMTPS id 20VJX9Ec064546 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Mon, 31 Jan 2022 11:33:09 -0800 (PST) (envelope-from glebius@freebsd.org) Received: (from glebius@localhost) by cell.glebi.us (8.16.1/8.16.1/Submit) id 20VJX86F064545; Mon, 31 Jan 2022 11:33:08 -0800 (PST) (envelope-from glebius@freebsd.org) X-Authentication-Warning: cell.glebi.us: glebius set sender to glebius@freebsd.org using -f Date: Mon, 31 Jan 2022 11:33:08 -0800 From: Gleb Smirnoff To: Marko Zec Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 91f44749c6fe - main - ifnet: make if_index global Message-ID: References: <202201270600.20R601qd059593@gitrepo.freebsd.org> <20220129162034.6cda27df@x25> List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220129162034.6cda27df@x25> X-Rspamd-Queue-Id: 4JndX23Xsdz3rCc X-Spamd-Bar: / Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=softfail (mx1.freebsd.org: 162.251.186.162 is neither permitted nor denied by domain of glebius@freebsd.org) smtp.mailfrom=glebius@freebsd.org X-Spamd-Result: default: False [0.86 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-0.99)[-0.991]; FREEFALL_USER(0.00)[glebius]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_DN_SOME(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; MIME_GOOD(-0.10)[text/plain]; HAS_XAW(0.00)[]; DMARC_NA(0.00)[freebsd.org]; R_SPF_SOFTFAIL(0.00)[~all:c]; NEURAL_SPAM_SHORT(0.96)[0.955]; TO_MATCH_ENVRCPT_SOME(0.00)[]; NEURAL_SPAM_LONG(1.00)[1.000]; MLMMJ_DEST(0.00)[dev-commits-src-all,dev-commits-src-main]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:27348, ipnet:162.251.186.0/24, country:US]; RCVD_COUNT_TWO(0.00)[2]; RCVD_TLS_ALL(0.00)[] X-ThisMailContainsUnwantedMimeParts: N Marko, On Sat, Jan 29, 2022 at 04:20:34PM +0100, Marko Zec wrote: M> > ifnet: make if_index global M> > M> > Now that ifindex is static to if.c we can unvirtualize it. For M> > lifetime of an ifnet its index never changes. To avoid leaking M> > foreign interfaces the net.link.generic.system.ifcount sysctl and the M> > ifnet_byindex() KPI filter their returned value on curvnet. Since M> > if_vmove() no longer changes the if_index, inline ifindex_alloc() and M> > ifindex_free() into if_alloc() and if_free() respectively. M> > M> > API wise the only change is that now minimum interface index can M> > be greater than 1. M> M> IMO this is a huge change in an API which has been stable like forever. M> Was this tested with a broader spectrum of userland consumers, such as M> FRR & alike? What exactly is FRR? If we see there is an expectation from some software that index shall start at 1, I will rethink this change. So far there is no such evidence. M> > The holes in interface indexes were always allowed. M> M> True, the holes were formally allowed but were never huge, and now they M> will be guaranteed with #vnets > 1. Again, that's not something 3rd M> party tools would expect. Well "huge" or "small" doesn't make difference for algorithms. Holes are allowed. M> What could be the repercussions of ifindex globalization to 16-bit size M> of ifru_index in struct ifreq? M> What can prevent a vnet to inflate the global ifindex space, causing M> the linear scan in sysctl_ifcount() to become a global problem? We don't think that virtualization of if_index was there to save if_index space, was it? I think the reason was just cause it was easier. M> Why do we now need to check wheter curvnet != NULL in ifnet_byindex(), M> i.e. are there callers to ifnet_byindex() where curvnet is not set? If M> so, that smells like a bug in the caller? Good point. Maybe an assertion should be put there. M> Or, is it the other way around, that ifnet_byindex() is now intended to M> become a silent workaround for callers which lack curvnet context? If M> that is so, it is indeed becoming a major VNET KPI feature (though I'd M> call it a bug), and as such, it should have been clearly stated in both M> the commit message, and as a comment in the ifnet_byindex() code. M> M> Finally, the commit message does not reveal any clues what this change M> actually tries to accomplish. The claim "Now that ifindex is static to M> if.c we can unvirtualize it." tell us nothing about the true intent. Well, the intent is revealed in the following commits, that use index as the only true stable & unique identifier of an interface except its pointer. I have had a previous version, that was maintaining two indexes a virtual and non-virtual: https://reviews.freebsd.org/D33265 If you ask me of a perfect version of this design, I would say same thing I already said in other topics: if_vmove shall not exist. If if_vmove didn't exist, a virtual index would provide stable and unique identifier for an interface. If interfaces were tied to their IFNET for their lifetime, so less problems would exist. I already lost count of problems created by if_vmove :( M> Sorry for chiming in late, but my sentiment against this change is M> obvious... I'm also sorry for not including you in the review. Please add yourself to https://reviews.freebsd.org/project/members/24/ -- Gleb Smirnoff