crossbow discuss - Jun 2008 - code review Brussels persistence

Recently discovered 6696737 caused us to reevaluate the already frowned 
upon attach(9E) time property access functions mac_prop_init/get. The 
conclusion was that we do not really need them, as long as the initial 
mc_setprop invocations occur before mc_start, which they do. This makes 
persistence "just work" for today''s brusselized drivers and
future
drivers will follow suit.

New webrev and cscoped workspace:

	http://cr.opensolaris.org/~artem/pers/
	/net/aja.sfbay/export0/brussels/br-pers-dls/

Good news for the local optimizers among us is that the diffs are even 
simpler and 35% slimmer, with the same great flavor.

Hopefully we can start converging now.

thanks,
-Artem


(old webrev moved to http://cr.opensolaris.org/~artem/pers-0514/)

Artem.Kachitchkin at Sun.COM said:
 > 
 > Recently discovered 6696737 caused us to reevaluate the already frowned 
 > upon attach(9E) time property access functions mac_prop_init/get. The 
 > conclusion was that we do not really need them, as long as the initial 
 > mc_setprop invocations occur before mc_start, which they do. This makes 
 > persistence "just work" for today''s brusselized drivers
and future
 > drivers will follow suit.
 > 
 > New webrev and cscoped workspace:
 > 
 > 	http://cr.opensolaris.org/~artem/pers/
 > 	/net/aja.sfbay/export0/brussels/br-pers-dls/
 > 
 > Good news for the local optimizers among us is that the diffs are even 
 > simpler and 35% slimmer, with the same great flavor.

A couple simple questions:

	* Can dls_prop_t use the C99 VLA support to avoid having to
	  subtract 1 when dealing with dls_prop_t''s?  Or does that
	  not work for some reason?

	* What uses PROP_HASHSZ in dls_prop.c?

	* Why are i_dls_prop_lock and i_dls_prop_list not `static''?

	* What in dls_prop.c makes use of <sys/door.h> or <sys/softmac.h>?
	  In general, looks like there some crufty #includes.

--
meem

Peter Memishian writes:
> 	* What in dls_prop.c makes use of <sys/door.h> or
<sys/softmac.h>?
> 	  In general, looks like there some crufty #includes.
As a side note (and perhaps to nobody in particular): when I create a
header file, and sometimes when I update one, I''ll create a
"foo.c"
file with one line looking something like this in it:

	#include <that_one_header.h>

I then compile foo.c, making sure that the header file can stand alone
(meaning that the user isn''t forced into adding extra #includes just
to use my header; which also means that it''s nicely order-
independent), and then I run lint with -axsNlevel=2 and check for
unnecessary #includes.

Good maintainability depends on good hygiene.  ;-}

-- 
James Carlson, Solaris Networking              <james.d.carlson at
sun.com>
Sun Microsystems / 35 Network Drive        71.232W   Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757   42.496N   Fax +1 781 442 1677

> > A couple simple questions:
 > >
 > > 	* Can dls_prop_t use the C99 VLA support to avoid having to
 > > 	  subtract 1 when dealing with dls_prop_t''s?  Or does that
 > > 	  not work for some reason?
 > 
 > Are C99 VLAs now blessed for general use in ON code?  Are there any 
 > implications or gotchas surrounding there use?

I tried to use VLAs for a private ioctl payload a little while back and
recall running into a problem, but now I can''t remember what it was :-(
Certainly, other C99 features are now being used in ON such as __VA_ARGS__.

--
meem

>  > 	http://cr.opensolaris.org/~artem/pers/
> 
> A couple simple questions:
> 
> 	* Can dls_prop_t use the C99 VLA support to avoid having to
> 	  subtract 1 when dealing with dls_prop_t''s?  Or does that
> 	  not work for some reason?
> 
> 	* What uses PROP_HASHSZ in dls_prop.c?
> 
> 	* Why are i_dls_prop_lock and i_dls_prop_list not `static''?
> 
> 	* What in dls_prop.c makes use of <sys/door.h> or
<sys/softmac.h>?
> 	  In general, looks like there some crufty #includes.
All great questions! Especially because it doesn''t take me a week to 
come up with the answers. You''re right on all accounts - will fix it.

One important thing I forgot to mention. The in-kernel property cache is 
technically no longer need: we always propagate properties from the 
ioctl straight into mc_setprop. I see two reasons to retain the cache:

- Observability/diagnostics. Using ::dls_prop to list loaded properties 
is so much easier than digging through each driver''s soft state
structure.

- Potentially useful for future projects (e.g. Crossbow).

Cons:

- a few dozen lines more code to maintain
- a little more kernel memory used

I''d rather keep it, but I don''t feel strongly about this (or
anything,
really, except peace of earth).

-Artem

On Mon, Jun 02, 2008 at 04:39:29PM -0400, Peter Memishian wrote:
>  > > 	* Can dls_prop_t use the C99 VLA support to avoid having to
>  > > 	  subtract 1 when dealing with dls_prop_t''s?  Or does
that
>  > > 	  not work for some reason?
>  > 
>  > Are C99 VLAs now blessed for general use in ON code?  Are there any 
>  > implications or gotchas surrounding there use?
> 
> I tried to use VLAs for a private ioctl payload a little while back and
> recall running into a problem, but now I can''t remember what it
was :-(
The CTF tools have problems. Last I heard, someone working on the fix

regards
john

On (06/02/08 14:25), Artem Kachitchkine wrote:
> One important thing I forgot to mention. The in-kernel property cache is 
> technically no longer need: we always propagate properties from the 
> ioctl straight into mc_setprop. I see two reasons to retain the cache:
> 
> - Observability/diagnostics. Using ::dls_prop to list loaded properties 
> is so much easier than digging through each driver''s soft state
structure.
> 
> - Potentially useful for future projects (e.g. Crossbow).
I also have an instinct that it should be kept, so that the mac
layer can know the property setting without having to consult the
driver at all times. Also, wasn''t this needed for autopush/zone
temporary settings?

I looked at your latest webrev, it looks good. Thanks for keeping
up with this so patiently!

We should document the lessons learned over the last few weeks
somewhere though, so that others don''t stumble on the same constraints
that we did. Although we could put this in the design doc, I think it''s
more likely to be useful as comments in the code, perhaps in the
vicinity of dls_devnet_set/dls_devnet_prop_load_task to explain why we
need to asynchronously do the property loading, and how it all fits
together. 

--Sowmini

> Also, wasn''t this needed for autopush/zone temporary settings?
Nope.

-Artem

> Artem.Kachitchkin at Sun.COM said:
>  > 
>  > Recently discovered 6696737 caused us to reevaluate the already
frowned
>  > upon attach(9E) time property access functions mac_prop_init/get. The
>  > conclusion was that we do not really need them, as long as the
initial
>  > mc_setprop invocations occur before mc_start, which they do. This
makes
>  > persistence "just work" for today''s brusselized
drivers and future
>  > drivers will follow suit.
>  > 
>  > New webrev and cscoped workspace:
>  > 
>  > 	http://cr.opensolaris.org/~artem/pers/
>  > 	/net/aja.sfbay/export0/brussels/br-pers-dls/
>  > 
>  > Good news for the local optimizers among us is that the diffs are
even
>  > simpler and 35% slimmer, with the same great flavor.
> 
Artem,

I didn''t look at the mdb related code, and below are my comments for
other
part of your code:

a. Did you consider the DR case? See comment 2 above dls_devnet_rename(). 
Note that because a ddp->dd_vlanid could change (because of this case):

   - dls_devnet_prop_load_task() needs lock protection when accessing the 
dd_vlanid field.

   - in the case 2 of dls_devnet_rename(), you will need to unload the 
link properties associated with the old linkid and reapplies the link 
properties associated with the new linkid.

b. dls_devnet_prop_load_task()

   why softmac_hold_device() is needed?

c. Since link properties are all initialized by the new mechanism, do we 
still need the dladm_init_linkprop() calls in various RCM modules and 
libdladm function calls (for example, i_dladm_aggr_up())?

d. Do we still need special handling for wifi devices in network-physical? 
Also, is it possible for "dladm init-linkprop" to take one specific
name
(which requires the changes in the do_init_linkprop() function)?

e. I don''t think we should to keep the link property values in the
kernel
and we can safely remove the whole dls_prop.c file, especially that 1) 
today there are no consumers for that, and 2) some of link properties 
simply do not go through the dls_set_prop() code path. If we need the 
kernel copy one day, it should be easy to add them back.

Thanks
- Cathy

> d. Do we still need special handling for wifi devices in network-physical? 
> Also, is it possible for "dladm init-linkprop" to take one
specific name
> (which requires the changes in the do_init_linkprop() function)?
Artem can confirm, but we''ll need this as long as the wifi drivers
are not converted to provide Brussels callbacks.. we''re working with
the wifi team to eliminate the wifi ioctls and replace them
with Brussels callbacks..
> e. I don''t think we should to keep the link property values in the
kernel
> and we can safely remove the whole dls_prop.c file, especially that 1) 
> today there are no consumers for that, and 2) some of link properties 
> simply do not go through the dls_set_prop() code path. If we need the 
> kernel copy one day, it should be easy to add them back.
I would still vote in favor of keeping them.. as Artem pointed out it 
helps with providing mdb support for finding what customizations
were done when we are looking at a crash dump. This was one of the
big complaints about ndd- that we can''t figure out this information
from a crash dump easily.

--Sowmini

Sowmini.Varadhan at Sun.COM wrote:
>> d. Do we still need special handling for wifi devices in
network-physical?
>> Also, is it possible for "dladm init-linkprop" to take one
specific name
>> (which requires the changes in the do_init_linkprop() function)?
> 
> Artem can confirm, but we''ll need this as long as the wifi drivers
> are not converted to provide Brussels callbacks.. we''re working
with
> the wifi team to eliminate the wifi ioctls and replace them
> with Brussels callbacks..
> 
I thought wifi link properties will also be initialized in the same code path?

     dls_prop_load()->dls_mgmt_linkprop_init()->
         dlmgmt_upcall_linkprop_init()->dladm_init_linkprop()
>> e. I don''t think we should to keep the link property values in
the kernel
>> and we can safely remove the whole dls_prop.c file, especially that 1) 
>> today there are no consumers for that, and 2) some of link properties 
>> simply do not go through the dls_set_prop() code path. If we need the 
>> kernel copy one day, it should be easy to add them back.
> 
> I would still vote in favor of keeping them.. as Artem pointed out it 
> helps with providing mdb support for finding what customizations
> were done when we are looking at a crash dump. This was one of the
> big complaints about ndd- that we can''t figure out this
information
> from a crash dump easily.
Like I said, some of the link properties are not in the kernel copy as it 
does not go through the same mac_init_prop() code path. If we are going to 
have this mdb tool, I would expect that I can get the complete picture, no 
matter the implementation details.

Thanks
- Cathy
> 
> --Sowmini
> 
> _______________________________________________
> crossbow-discuss mailing list
> crossbow-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss

On (06/04/08 17:52), Cathy Zhou wrote:
> I thought wifi link properties will also be initialized in the same code
path?
>
>     dls_prop_load()->dls_mgmt_linkprop_init()->
>         dlmgmt_upcall_linkprop_init()->dladm_init_linkprop()
>
that''s true, I''d forgotten about that.
> Like I said, some of the link properties are not in the kernel copy as it 
> does not go through the same mac_init_prop() code path. If we are going 
which properties are these? If you are referring to zone and autopush,
we are working on fixing these at the moment so these will be covered
by the mdb macros soon.

--Sowmini

Sowmini.Varadhan at Sun.COM wrote:
>> d. Do we still need special handling for wifi devices in
network-physical?
>> Also, is it possible for "dladm init-linkprop" to take one
specific name
>> (which requires the changes in the do_init_linkprop() function)?
> 
> Artem can confirm, but we''ll need this as long as the wifi drivers
> are not converted to provide Brussels callbacks.. we''re working
with
> the wifi team to eliminate the wifi ioctls and replace them
> with Brussels callbacks..
dladm_init_linkprop() will issue the right ioctls for wifi. We used to 
care about the ioctl initially, when we tapped into SETPROP for caching, 
but with the elimination of mac_prop_init, I don''t think we care
anymore
or need this line in network-physical.
>> e. I don''t think we should to keep the link property values in
the kernel
>> and we can safely remove the whole dls_prop.c file, especially that 1) 
>> today there are no consumers for that, and 2) some of link properties 
>> simply do not go through the dls_set_prop() code path. If we need the 
>> kernel copy one day, it should be easy to add them back.
> 
> I would still vote in favor of keeping them.. as Artem pointed out it 
> helps with providing mdb support for finding what customizations
> were done when we are looking at a crash dump. This was one of the
> big complaints about ndd- that we can''t figure out this
information
> from a crash dump easily.
I can bend either way. At this point, I am agreeing to anything ;)

-Artem

> a. Did you consider the DR case? See comment 2 above dls_devnet_rename(). 
> Note that because a ddp->dd_vlanid could change (because of this case):
> 
>    - dls_devnet_prop_load_task() needs lock protection when accessing the 
> dd_vlanid field.
> 
>    - in the case 2 of dls_devnet_rename(), you will need to unload the 
> link properties associated with the old linkid and reapplies the link 
> properties associated with the new linkid.
I''ll look into it.
> b. dls_devnet_prop_load_task()
> 
>    why softmac_hold_device() is needed?
I guess it''s not. Will remove.
> c. Since link properties are all initialized by the new mechanism, do we 
> still need the dladm_init_linkprop() calls in various RCM modules and 
> libdladm function calls (for example, i_dladm_aggr_up())?
Will file a CR.
> d. Do we still need special handling for wifi devices in network-physical? 
We probably don''t. Will remove.
> Also, is it possible for "dladm init-linkprop" to take one
specific name
> (which requires the changes in the do_init_linkprop() function)?
Yes, the code is already there.
> e. I don''t think we should to keep the link property values in the
kernel
> and we can safely remove the whole dls_prop.c file, especially that 1) 
> today there are no consumers for that, and 2) some of link properties 
> simply do not go through the dls_set_prop() code path. If we need the 
> kernel copy one day, it should be easy to add them back.
OK.

-Artem

Artem Kachitchkine wrote:
> 
>> a. Did you consider the DR case? See comment 2 above 
>> dls_devnet_rename(). Note that because a ddp->dd_vlanid could change
>> (because of this case):
>>
>>    - dls_devnet_prop_load_task() needs lock protection when accessing 
>> the dd_vlanid field.
>>
>>    - in the case 2 of dls_devnet_rename(), you will need to unload the 
>> link properties associated with the old linkid and reapplies the link 
>> properties associated with the new linkid.
> 
> I''ll look into it.
> 
>> c. Since link properties are all initialized by the new mechanism, do 
>> we still need the dladm_init_linkprop() calls in various RCM modules 
>> and libdladm function calls (for example, i_dladm_aggr_up())?
> 
> Will file a CR.
> 
Why don''t fix it now?
>> Also, is it possible for "dladm init-linkprop" to take one
specific
>> name (which requires the changes in the do_init_linkprop() function)?
> 
> Yes, the code is already there.
> 
My question was: when dladm init-linkprop takes a specific link name?

Another problem I just think about. Do you need something similar to 
dls_devnet_prop_complete() when mac_open_by_linkid() is called? Or 
probably in dls_devnet_hold_tmp()?

Thanks
- Cathy

>>> c. Since link properties are all initialized by the new mechanism,
do
>>> we still need the dladm_init_linkprop() calls in various RCM
modules
>>> and libdladm function calls (for example, i_dladm_aggr_up())?
>>
>> Will file a CR.
>>
> Why don''t fix it now?
Because it will require more code reviewers, more testing, delay 
integration even further and upset our very patient management. These 
calls do not hurt, they are just extra fat that can be removed later.
>>> Also, is it possible for "dladm init-linkprop" to take
one specific
>>> name (which requires the changes in the do_init_linkprop()
function)?
>>
>> Yes, the code is already there.
>>
> My question was: when dladm init-linkprop takes a specific link name?
When invoked manually or via a custom script. Generally useful to 
developers like me.
> Another problem I just think about. Do you need something similar to 
> dls_devnet_prop_complete() when mac_open_by_linkid() is called? Or 
> probably in dls_devnet_hold_tmp()?
I don''t know... Do we?

-Artem

Sowmini.Varadhan at Sun.COM wrote:
> On (06/04/08 17:52), Cathy Zhou wrote:
>> I thought wifi link properties will also be initialized in the same
code path?
>>
>>     dls_prop_load()->dls_mgmt_linkprop_init()->
>>         dlmgmt_upcall_linkprop_init()->dladm_init_linkprop()
>>
> 
> that''s true, I''d forgotten about that.
> 
>> Like I said, some of the link properties are not in the kernel copy as
it
>> does not go through the same mac_init_prop() code path. If we are going
> 
> which properties are these? If you are referring to zone and autopush,
> we are working on fixing these at the moment so these will be covered
> by the mdb macros soon.
> 
Yes, zone autopush and all wifi link properties. I guess I don''t feel 
strong about this, as long as we will fix the problem.

Thanks
- Cathy

> >>> c. Since link properties are all initialized by the new
mechanism, do
 > >>> we still need the dladm_init_linkprop() calls in various RCM
modules
 > >>> and libdladm function calls (for example, i_dladm_aggr_up())?
 > >>
 > >> Will file a CR.
 > >>
 > > Why don''t fix it now?
 > 
 > Because it will require more code reviewers, more testing, delay 
 > integration even further and upset our very patient management. These 
 > calls do not hurt, they are just extra fat that can be removed later.

I disagree.  Those calls will lead to confusion.  Part of adding a proper
persistence model for properties includes removing the old incomplete
mechanism.

-- 
meem

Peter Memishian wrote:
>  > >>> c. Since link properties are all initialized by the new
mechanism, do
>  > >>> we still need the dladm_init_linkprop() calls in various
RCM modules
>  > >>> and libdladm function calls (for example,
i_dladm_aggr_up())?
>  > >>
>  > >> Will file a CR.
>  > >>
>  > > Why don''t fix it now?
>  > 
>  > Because it will require more code reviewers, more testing, delay 
>  > integration even further and upset our very patient management. These
>  > calls do not hurt, they are just extra fat that can be removed later.
> 
> I disagree.  Those calls will lead to confusion.  Part of adding a proper
> persistence model for properties includes removing the old incomplete
> mechanism.
OK. I''d surely hate to cause confusion in anyone, so I will remove the 
old incomplete mechanism.

thanks,
-Artem

>> My question was: when dladm init-linkprop takes a specific link name?
> 
> When invoked manually or via a custom script. Generally useful to 
> developers like me.
> 
Okay, I understand that it can be used by developer for diagnosis purpose. 
But are we going to to include init-linkprop in the man page and make it 
visible to the customer?
>> Another problem I just think about. Do you need something similar to 
>> dls_devnet_prop_complete() when mac_open_by_linkid() is called? Or 
>> probably in dls_devnet_hold_tmp()?
> 
> I don''t know... Do we?
> 
Well. I think we need to carefully look at each link property and see 
whether it should to be initialized when a MAC client opens a mac. For 
example, when aggregation aggregates several ethernet links whose MTU is 
set to 9000, whether this aggregation''s MTU should also be set to 9000
as
well?

Thanks
- Cathy
> -Artem
> _______________________________________________
> networking-discuss mailing list
> networking-discuss at opensolaris.org

http://cr.opensolaris.org/~artem/pers/
	/net/aja.sfbay/export0/brussels/br-pers-dls/

Updated. I''ve been working offline with Cathy, she seems okay with the 
state of DLS now. Calls to dladm_init_linkprop() have been removed from 
RCM and other weird places. GLDv3 tests pass.

thanks,
-Artem

Hi artem,

I just have one minor naming nit.
can you change dls_devnet_prop_complete() to 
dls_devnet_prop_task_wait()? it''s unclear what
''complete'' means.

with the above change, dls_devnet_prop_load_task() should also be 
changed to dls_devnet_prop_task() to be consistent and to show 
relationship between the two functions.

thanks
eric


Artem Kachitchkine wrote:
> 	http://cr.opensolaris.org/~artem/pers/
> 	/net/aja.sfbay/export0/brussels/br-pers-dls/
> 
> Updated. I''ve been working offline with Cathy, she seems okay with
the
> state of DLS now. Calls to dladm_init_linkprop() have been removed from 
> RCM and other weird places. GLDv3 tests pass.
> 
> thanks,
> -Artem
> _______________________________________________
> networking-discuss mailing list
> networking-discuss at opensolaris.org

Eric Cheng wrote:
> Hi artem,
> 
> I just have one minor naming nit.
> can you change dls_devnet_prop_complete() to 
> dls_devnet_prop_task_wait()? it''s unclear what
''complete'' means.
> 
> with the above change, dls_devnet_prop_load_task() should also be 
> changed to dls_devnet_prop_task() to be consistent and to show 
> relationship between the two functions.
Sure, I''ll change it.

-Artem