From owner-freebsd-current@FreeBSD.ORG Tue Sep 9 17:59:45 2014 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 717689E2; Tue, 9 Sep 2014 17:59:45 +0000 (UTC) Received: from mail-la0-x22e.google.com (mail-la0-x22e.google.com [IPv6:2a00:1450:4010:c03::22e]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id A6895EAD; Tue, 9 Sep 2014 17:59:44 +0000 (UTC) Received: by mail-la0-f46.google.com with SMTP id pv20so20022112lab.19 for ; Tue, 09 Sep 2014 10:59:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=mD0Rid2F3C1x7UOP1UGEsDEu+GsToRa4VVbnS8+QS6s=; b=QgWSGNKOFG2TCGwK2Fk8ixPvFmn8ajyoAO3FxDfSuizDfYZivcuw8aqceRgvP2AD9W w8Xk8tt4ovIiB7vVQ2UdweY43Wip6X4dzkV3NNPfbX7fqW5AAwXS0vPdlCiLcJKxk42t qDUi7njCBI27gUXdxDoSzOeum0/2aNSnzwhXJs/MevrqmULZtj1wCJ0WaqXoFM93GIy8 GdDJTHxPO8n6Sv3S7N4Pps9T0U1UhY0PpH9DrcBMMvvwOdONV53AdRsXFCCB0jPYEFPF /fK1miaCLukju++0mPl3LQPNbmvAzHPDGWxiqnD2FI/KXdZBDbyfb01wUqMz1uF0/yjZ f3rQ== MIME-Version: 1.0 X-Received: by 10.152.9.170 with SMTP id a10mr11379115lab.79.1410285582415; Tue, 09 Sep 2014 10:59:42 -0700 (PDT) Sender: rizzo.unipi@gmail.com Received: by 10.114.26.37 with HTTP; Tue, 9 Sep 2014 10:59:42 -0700 (PDT) In-Reply-To: <20140909121708.GE17059@glebius.int.ru> References: <20140909103719.GB17059@glebius.int.ru> <20140909121708.GE17059@glebius.int.ru> Date: Tue, 9 Sep 2014 19:59:42 +0200 X-Google-Sender-Auth: KDyWyLqUEbugvxsa_Ggl2nyXhZM Message-ID: Subject: Re: RFC: please put back spare fields in struct ifnet (removed in svn 270870) From: Luigi Rizzo To: Gleb Smirnoff Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.18-1 Cc: George Neville-Neil , FreeBSD Current , Stefano Garzarella X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Sep 2014 17:59:45 -0000 On Tue, Sep 9, 2014 at 2:17 PM, Gleb Smirnoff wrote: > Luigi, > > On Tue, Sep 09, 2014 at 01:01:13PM +0200, Luigi Rizzo wrote: > L> > The harm is obvious: someone commits code that _uses_ spare field > L> > without assigning it a new name. Spare field is a placeholder. Of > L> > course you can use it while you experiment. However, I don't see > L> > problem with patching your source tree where you experiment. > L> > L> The problem with _not_ having spares is that you have to recompile > L> everything after patching the headers. With the spares, i could > L> make netmap a simple add-on kernel module with no dependencies. > L> Same on linux for what matters (there wasn't a spare there, but > L> nobody used ax25 and i could check and reuse it). > > What if another kernel hacker comes and also uses if_pspare[0] for > his own super-duper feature? What would you do, if then a user > reports that enabling netmap on his box instapanics if he had > some other feature turned on? > =E2=80=8Bi'll live with that if there is contention and the use of the pspare is not checked (in linux i do check that the pointer is not used before trying to use it). With the removal of the spares we remove the risk but also prevent people from loading the module without recompiling the whole kernel. That is a bit too radical as a path to safety. It's like saying that rather than seat belts and airbags we should not drive to reduce our chances of being involved in a car accident. > L> > The ABI plan for 'struct ifnet' is that the struct becomes > L> > opaque for device drivers. So its size and alignment no longer > L> > matters. Those who want to add new fields to struct ifnet, > L> > would also need to add accessor methods. Bits of this plan > L> > are already committed by Marcel, but its only first step. > L> > L> =E2=80=8Bspare fields <-> spare accessors=E2=80=8B > > Let me repeat: in future device drivers will not be capable > to dereference struct ifnet. So, any feature pointers would > be accessed via functions, making struct ifnet no longer part > of ABI. This hasn't yet happened, but one already can (and > should!) use accessors in any new code. > > let me repeat too :) My point is preserving support for out of tree modules, and spares (or spare accessors, or the ABI you mention below; something that gets you quickly a vendor specific pointer from an opaque ifnet) were useful for that. I think the removal of spares should have happened together with the commit of the new mechanism. If it is ready now, let's move with it and be done with this discussion. If not, I would like to bring back the pspares with a big note summarizing this discussion, and remove then when the new mechanism is ready. If i read correctly your comment below about the "properly named placeholder" you seem to be ok with that ? cheers luigi L> > I'm afraid that if fields are there back, the situation that > L> > happened with netmap (use of spare field) would repeat. > L> > L> =E2=80=8Bthe spare pointer used by netmap was clearly indicated by a c= omment, > L> and giving it a dedicated name instead of if_pspare[0] was > L> expected to happen (hopefully together with a __FreeBSD_version bump, > L> which has not happened). > > I don't agree that /* 1 netmap, 7 TDB */ is a clear comment. I > understood as "in future one of those is to be used for netmap". > > Regarding __FreeBSD_version: the field should have been renamed > in the commit that brought in netmap, not in my commit. My commit > removed a field that must not be used, so it is not a reason > to bump. > > If you need closest __FreeBSD_version for the change, then > please use 1100030, it happened next day. > > L> I am not worried that the name change was missed when deleting > if_pspare[]. > L> Mistakes happen and the error was promptly corrected (apart from the > L> version bump). > L> > L> What worries me is losing some flexibility in dealing with > L> out-of-tree kernel modules. > > Just put a properly named placeholder for them as a quick solution. > > A nicer solution would be to add an API for storing vendor-specific > pointers on ifnet, providing a cookie. I'm discussing that now with > kmacy@, who also thinks in that direction. > > -- > Totus tuus, Glebius. > --=20 -----------------------------------------+------------------------------- Prof. Luigi RIZZO, rizzo@iet.unipi.it . Dip. di Ing. dell'Informazione http://www.iet.unipi.it/~luigi/ . Universita` di Pisa TEL +39-050-2211611 . via Diotisalvi 2 Mobile +39-338-6809875 . 56122 PISA (Italy) -----------------------------------------+-------------------------------