Re: RFC: please put back spare fields in struct ifnet (removed in svn 270870)

2014-09-10 Thread Gleb Smirnoff
  Luigi,

On Tue, Sep 09, 2014 at 07:59:42PM +0200, Luigi Rizzo wrote:
L My point is preserving support for out of tree modules,
L and spares (or spare accessors, or the ABI you mention below;
L something that gets you quickly a vendor specific pointer
L from an opaque ifnet) were useful for that.
L 
L I think the removal of spares should have happened together
L with the commit of the new mechanism. If it is ready now,
L let's move with it and be done with this discussion.
L 
L If not, I would like to bring back the pspares
L with a big note summarizing this discussion,
L and remove then when the new mechanism is ready.
L If i read correctly your comment below about
L the properly named placeholder you seem to be ok with that ?

It would be absolutely okay if you commit right now a properly
named placeholder for your new subsystem, that you work on right
now. With the proper name no one will unintentionally hijack it.
Would this be a satisfiable solution for you?

The suggested ABI mechanism is still under discussion and
development.

-- 
Totus tuus, Glebius.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org


Re: RFC: please put back spare fields in struct ifnet (removed in svn 270870)

2014-09-09 Thread Gleb Smirnoff
  Luigi,

On Tue, Sep 09, 2014 at 12:13:42PM +0200, Luigi Rizzo wrote:
L svn 270870 removed all the if_*spare fields in struct ifnet.
L They are replaced with the following comment
L 
L /*
L  * Spare fields to be added before branching a stable branch, so
L  * that structure can be enhanced without changing the kernel
L  * binary interface.
L  */
L 
L ​which leaves me a bit unhappy.
L Having a stable ABI is useful not only for stable branches,
L but also (I would say even more) with head, so people can
L run experimental code with limited modifications to the sources.
L 
L Cases in point:
L - we used one spare field extensively when experimenting
L   with netmap, and being able to just build a module without having
L   to recompile the whole kernel was a big win.
L - we are developing some software GSO and again it was great to have
L   the spares in the tcpcb and in the ifnet so we could limit
L   modifications to headers used by multiple sources.
L 
L I would kindly suggest to put the spares back.
L I can't see how they can possibly harm.

The harm is obvious: someone commits code that _uses_ spare field
without assigning it a new name. Spare field is a placeholder. Of
course you can use it while you experiment. However, I don't see
problem with patching your source tree where you experiment.

The ABI plan for 'struct ifnet' is that the struct becomes
opaque for device drivers. So its size and alignment no longer
matters. Those who want to add new fields to struct ifnet,
would also need to add accessor methods. Bits of this plan
are already committed by Marcel, but its only first step.

I'm afraid that if fields are there back, the situation that
happened with netmap (use of spare field) would repeat.

-- 
Totus tuus, Glebius.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org

Re: RFC: please put back spare fields in struct ifnet (removed in svn 270870)

2014-09-09 Thread Luigi Rizzo
On Tue, Sep 9, 2014 at 12:37 PM, Gleb Smirnoff gleb...@freebsd.org wrote:

   Luigi,

 On Tue, Sep 09, 2014 at 12:13:42PM +0200, Luigi Rizzo wrote:
 L svn 270870 removed all the if_*spare fields in struct ifnet.
 L They are replaced with the following comment
 L
 L /*
 L  * Spare fields to be added before branching a stable branch, so
 L  * that structure can be enhanced without changing the kernel
 L  * binary interface.
 L  */
 L
 L ​which leaves me a bit unhappy.
 L Having a stable ABI is useful not only for stable branches,
 L but also (I would say even more) with head, so people can
 L run experimental code with limited modifications to the sources.
 L
 L Cases in point:
 L - we used one spare field extensively when experimenting
 L   with netmap, and being able to just build a module without having
 L   to recompile the whole kernel was a big win.
 L - we are developing some software GSO and again it was great to have
 L   the spares in the tcpcb and in the ifnet so we could limit
 L   modifications to headers used by multiple sources.
 L
 L I would kindly suggest to put the spares back.
 L I can't see how they can possibly harm.

 The harm is obvious: someone commits code that _uses_ spare field
 without assigning it a new name. Spare field is a placeholder. Of
 course you can use it while you experiment. However, I don't see
 problem with patching your source tree where you experiment.


The problem with _not_ having spares is that you have to recompile
everything after patching the headers. With the spares, i could
make netmap a simple add-on kernel module with no dependencies.
Same on linux for what matters (there wasn't a spare there, but
nobody used ax25 and i could check and reuse it).

Of course you can easily emulate extension fields for stuff that
is not accessed frequently (tough a version number would help),
but there are cases when you do need the extra info on a per-packet
basis.


 The ABI plan for 'struct ifnet' is that the struct becomes
 opaque for device drivers. So its size and alignment no longer
 matters. Those who want to add new fields to struct ifnet,
 would also need to add accessor methods. Bits of this plan
 are already committed by Marcel, but its only first step.


​spare fields - spare accessors​


 I'm afraid that if fields are there back, the situation that
 happened with netmap (use of spare field) would repeat.


​the spare pointer used by netmap was clearly indicated by a comment,
and giving it a dedicated name instead of if_pspare[0] was
expected to happen (hopefully together with a __FreeBSD_version bump,
which has not happened).

I am not worried that the name change was missed when deleting if_pspare[].
Mistakes happen and the error was promptly corrected (apart from the
version bump).

What worries me is losing some flexibility in dealing with
out-of-tree kernel modules.

​cheers
luigi​
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org

Re: RFC: please put back spare fields in struct ifnet (removed in svn 270870)

2014-09-09 Thread Gleb Smirnoff
  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?

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 ​spare fields - spare accessors​

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.

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 ​the spare pointer used by netmap was clearly indicated by a comment,
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.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org

Re: RFC: please put back spare fields in struct ifnet (removed in svn 270870)

2014-09-09 Thread Luigi Rizzo
On Tue, Sep 9, 2014 at 2:17 PM, Gleb Smirnoff gleb...@freebsd.org 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?


​i'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 ​spare fields - spare accessors​

 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 ​the spare pointer used by netmap was clearly indicated by a comment,
 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.




-- 
-+---
 Prof. Luigi RIZZO, ri...@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)
-+---
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org