crossbow discuss - Apr 2008 - code review Brussels persistence

Testing is still in progress, minor changes might still happen and debug 
printf''s need to be removed, but overall it is ready for review.

	http://cr.opensolaris.org/~artem/pers-0421/

Anyone is welcome to provide constructive comments, gentle critique, 
witty remarks and naughty limericks. Following folks might be especially 
interested:

Cathy and/or Seb: dlmgmtd, dls
Eric Cheng and/or Michael Lim: mac, dld, dls
Ted You: bge
Sowmini: mac, dld, libdladm, dladm, mdb

I''d appreciate your inputs before next Tuesday, April 29th.

thanks,
-Artem

Artem,

Looks good overall!

here''s my feedback (if a file''s not listed, I have no specific
comments on it).

--Sowmini
---------------------------------------------------------------------------

usr/src/cmd/svc/milestone/net-physical
  - Don''t we still need lines 209-218 for wifi drivers?

usr/src/uts/common/sys/mac_impl.h
  - Use DLD_LINKPROP_NAME_MAX for mp_name?

usr/src/uts/common/io/mac/mac.c
  - printf at line 3000 can be removed. 
  - question: what happens if err at line 2947 is EBUSY (e.g., for
    mtu, where driver cannot apply the property just yet)? don''t
    we want to do_update in this case, so that the driver can pick
    up the setting next time?
  - mac_prop_update() should return an error if kmem_zalloc() fails at
    line 3184 (and trickle that error back up from mac_set_prop()
    and mac_set_prop_mac_only())
  - line 3038, check for null return from kmem_zalloc()

usr/src/uts/common/io/dld/dld_drv.c
  - drv_ioc_prop_common(): if I do a GETPROP and incorrectly set 
    DLD_PROP_FLAG_MAC_ONLY then the code would miss taking the locks.
    We probably don''t need to test for this explicitly since
we''d only
    get here from libdladm, but might be good to  add an
           ASSERT(!get || !mac_only);
    after line 534.

usr/src/lib/libdladm/common/libdladm.h
  - add comments about  DLADM_OPT_PROP_MAC_ONLY/DLADM_OPT_PROP_MAC_NAME.
    In terms of their interaction with mac_prop_load(),
    the distinction between the 2 is a bit confusing since both of them
    have the effect of not invoking the driver''s setprop routine.
    Are 2 flags needed? It is especially confusing that 
    setting MAC_NAME will result in mac_set_prop_mac_only() which is
    not obvious from the constant''s name.  Would it be cleaner if
    dlmgmtd mapped the MAC_NAME to MAC_ONLY with some comments explaining
    why this was being done?
    
usr/src/uts/common/io/bge/bge_main2.c
- printfs at line 3085, 3091  can be removed (make this a DTRACE_PROBE
  instead?)
- merge note: the uint64_t mtu will be a uint32_t after the nddcompat
  fixes.

> usr/src/cmd/svc/milestone/net-physical
>   - Don''t we still need lines 209-218 for wifi drivers?
Good question. If we have any kind of persistence for wifi drivers, it 
seems to be working by accident. That is, it relies on drivers not being 
unloaded, and the system is free to do so at any moment. And on DEBUG 
builds, will do so every 60 seconds: so even with this line present, 
when I set speed to 54, reboot and do dladm connect-wifi, then dladm 
show-linkprop still shows 36. I guess the question boils down to, should 
we preserve something that works by accident? If so, I can add an option 
for dladm init-linkprop, like -w or -m wifi, to only initialize 
properties for wireless links, and use this option in net-physical.
> usr/src/uts/common/io/mac/mac.c
>   - question: what happens if err at line 2947 is EBUSY (e.g., for
>     mtu, where driver cannot apply the property just yet)? don''t
>     we want to do_update in this case, so that the driver can pick
>     up the setting next time?
We won''t do update when err is EBUSY because of:

	do_update = (err == 0);

>   - mac_prop_update() should return an error if kmem_zalloc() fails at
>     line 3184 (and trickle that error back up from mac_set_prop()
>     and mac_set_prop_mac_only())
>   - line 3038, check for null return from kmem_zalloc()
kmem_zalloc never fails when called with KM_SLEEP.
> usr/src/lib/libdladm/common/libdladm.h
>   - add comments about  DLADM_OPT_PROP_MAC_ONLY/DLADM_OPT_PROP_MAC_NAME.
>     In terms of their interaction with mac_prop_load(),
>     the distinction between the 2 is a bit confusing since both of them
>     have the effect of not invoking the driver''s setprop routine.
>     Are 2 flags needed? It is especially confusing that 
>     setting MAC_NAME will result in mac_set_prop_mac_only() which is
>     not obvious from the constant''s name.  Would it be cleaner if
>     dlmgmtd mapped the MAC_NAME to MAC_ONLY with some comments explaining
>     why this was being done?
This been bothering me as well, especially since dld will return EINVAL 
when only one of two flags is set. Let me think about it, it feels like 
we can do fine with just one flag.

Other comments: agree and will fix.

-Artem

On (04/22/08 11:48), Artem Kachitchkine wrote:
>>   - Don''t we still need lines 209-218 for wifi drivers?
>
> Good question. If we have any kind of persistence for wifi drivers, it  
> seems to be working by accident. That is, it relies on drivers not being  
> unloaded, and the system is free to do so at any moment. And on DEBUG  
> builds, will do so every 60 seconds: so even with this line present,  
> when I set speed to 54, reboot and do dladm connect-wifi, then dladm  
> show-linkprop still shows 36. I guess the question boils down to, should  
> we preserve something that works by accident? If so, I can add an option  
> for dladm init-linkprop, like -w or -m wifi, to only initialize  
> properties for wireless links, and use this option in net-physical.
I think there may be some value in leaving it there, at least until
we fix more drivers (the br-pers only has fixes for bge, but nv
has Brussels support for nge, e1000g, nxge).
>> usr/src/uts/common/io/mac/mac.c
>>   - question: what happens if err at line 2947 is EBUSY (e.g., for
>>     mtu, where driver cannot apply the property just yet)?
don''t
>>     we want to do_update in this case, so that the driver can pick
>>     up the setting next time?
>
> We won''t do update when err is EBUSY because of:
>
> 	do_update = (err == 0);
I''m suggesting that we should do_update in this case, or provide
some way of letting the user update the persistent configuration 
so that the setting is picked up on the next restart.


--Sowmini

> I think there may be some value in leaving it there, at least until
> we fix more drivers (the br-pers only has fixes for bge, but nv
> has Brussels support for nge, e1000g, nxge).
I''ll see about adding the option to dladm init-linkprop then.
>> We won''t do update when err is EBUSY because of:
>>
>> 	do_update = (err == 0);
> 
> I''m suggesting that we should do_update in this case,
No, the cached value should always remain in sync with the driver''s
value.
> or provide
> some way of letting the user update the persistent configuration 
> so that the setting is picked up on the next restart.
''err'' will be propagated back to userland. dladm will print
the right
message. When this happens automatically via dlmgmtd, there''s nothing
we
can do. Spamming console or syslog is not the right thing either. OTOH, 
this is really a corner case: would happen for mtu in some cases, and 
with the drivers converted to use mac_prop_get(), the mtu will be set 
correctly.

-Artem

Artem Kachitchkine wrote:
> Testing is still in progress, minor changes might still happen and debug 
> printf''s need to be removed, but overall it is ready for review.
> 
> 	http://cr.opensolaris.org/~artem/pers-0421/
> 
> Anyone is welcome to provide constructive comments, gentle critique, 
> witty remarks and naughty limericks. Following folks might be especially 
> interested:
> 
> Cathy and/or Seb: dlmgmtd, dls
> Eric Cheng and/or Michael Lim: mac, dld, dls
> Ted You: bge
> Sowmini: mac, dld, libdladm, dladm, mdb
> 
> I''d appreciate your inputs before next Tuesday, April 29th.
> 
Hi Artem,

I haven''t done my reviewing yet, but I have several general questions
and
would like some clarification before I continue my review:

1. Can you think of any non-physical link driver (or even softmac) could call 
mac_prop_init()s? I ask this question because it is impossible for pseudo 
links (aggr, VNIC, or even softmac) to provide the instance argument. That 
number is assigned by the GLDv3 framework.

2. Assume the code path
mac_start()->mac_prop_load()->dls_mgmt_linkprop_init()->
dlmgmt_upcall_linkprop_init()->dlmgmt_getlink_by_dev() ...

Note that dlmgmt_getlink_by_dev() can only be used for physical links, and 
even for physical links, the device name is not the same as the mac name (for 
example, softmacs). Therefore, when dlmgmt_getlink_by_dev() is called with 
mac_name, it will return ENOENT for softmacs and other non-physical links.

The similar problem exists in dld_set_public_prop(). It assumes that mac name 
is the same as the device name.

3. I assume that you know that mac_prop_init() could fail when a new network 
interface is plugged in or you fresh-install a system. I guess since 
mac_prop_init() is used to apply the persistent link property configuration, 
those failures do not matter. But is a console message needed here if that 
fails (the bge_attach() function, line 3091)?

4. I don''t see dladm init-linkprop in network/physical service anymore.
How
link properties like autopush can be applied after your changes?

Below are some of my code review comments. Again, note that I haven''t
finish
looking at all the code yet.

mac.c

- It does not look right to call mac_prop_update() for the ENOTSUP case (line 
2952).

- mac_get_prop() does not seem to be symmetric to mac_set_prop(). I guess the 
reason that DLD_PROP_FLAG_MAC_ONLY only applies to mac_set_prop() is because 
in that case, the property is already initialized in the underlying driver. 
But the current flag name is misleading because it seems that this property 
only needs to be known by the mac layer. Can we change the name of 
DLD_PROP_FLAG_MAC_ONLY to something like DLD_PROP_FLAG_DRIVER_IS_SET?

linkprop.c

- In dladm_init_linkprop() function, it does not seem right to take (flags | 
DLADM_OPT_PERSIST) as the last argument. Note the possible flags value here is 
either DLADM_OPT_PROP_MAC_ONLY or DLADM_OPT_PROP_MAC_NAME.

Thanks
- Cathy

Cathy,
> 1. Can you think of any non-physical link driver (or even softmac) could 
> call mac_prop_init()s? I ask this question because it is impossible for 
> pseudo links (aggr, VNIC, or even softmac) to provide the instance 
> argument. That number is assigned by the GLDv3 framework.
This was discussed before and during ARC review and we feel that right
now mac_prop_init/get will be used by physical drivers only. The need
for drivers to actively retrieve properties is currently very limited,
mostly due to some drivers'' design defects/peculiarities. Most drivers
should be able to live without these functions, relying on mc_setprop
callbacks only.
> 2. Assume the code path 
> mac_start()->mac_prop_load()->dls_mgmt_linkprop_init()->
> dlmgmt_upcall_linkprop_init()->dlmgmt_getlink_by_dev() ...
> 
> Note that dlmgmt_getlink_by_dev() can only be used for physical links, 
> and even for physical links, the device name is not the same as the mac 
> name (for example, softmacs). Therefore, when dlmgmt_getlink_by_dev() is 
> called with mac_name, it will return ENOENT for softmacs and other 
> non-physical links.
> 
> The similar problem exists in dld_set_public_prop(). It assumes that mac 
> name is the same as the device name.
Yes, I think this also chimes with Meem''s comment about passing linkid
instead of name. It would be cleaner. I need to do some research. One
aspect that bothers me is interaction of dlmgmtd with partially attached
devices - i.e. the attach(9E)->mac_prop_init->mac_prop_load->... chain.
   Do you know of any existing kernel functions to translate macnames
into devnames and vice versa?
> 3. I assume that you know that mac_prop_init() could fail when a new 
> network interface is plugged in or you fresh-install a system. I guess 
> since mac_prop_init() is used to apply the persistent link property 
> configuration, those failures do not matter. But is a console message 
> needed here if that fails (the bge_attach() function, line 3091)?
I personally don''t see the need for console spam here. There''s
no
messages when ddi_prop_get_int() doesn''t find the property, why should
mac_prop_get() be different. I asked Ted to review, maybe he can provide
his opinion also.
> 4. I don''t see dladm init-linkprop in network/physical service
anymore.
> How link properties like autopush can be applied after your changes?
Well, this what this RFE is all about: to remove the need to follow each
ifconfig plumb with dladm init-linkprop. When a link is plumbed, MAC
start will trigger dladm_init_linkprop() and all these properties,
including autopush, will be automatically sent to the kernel. I tested
autopush, among others - it works.

Now, as Sowmini noted, we still need dladm init-linkprop to push
properties _after plumb_ for wireless drivers. So I''m currently adding
an option to initialize wifi properties only, what you''ll see in 
network/physical is:

	dladm init-linkprop -w
> mac.c
> 
> - It does not look right to call mac_prop_update() for the ENOTSUP case 
> (line 2952).
Will fix.
> - mac_get_prop() does not seem to be symmetric to mac_set_prop(). I 
> guess the reason that DLD_PROP_FLAG_MAC_ONLY only applies to 
> mac_set_prop() is because in that case, the property is already 
> initialized in the underlying driver. But the current flag name is 
> misleading because it seems that this property only needs to be known by 
> the mac layer. Can we change the name of DLD_PROP_FLAG_MAC_ONLY to 
> something like DLD_PROP_FLAG_DRIVER_IS_SET?
I feel DLD_PROP_FLAG_DRIVER_IS_SET is opaque. Naming is subjective,
it''s
a matter of perception. Luckily, we don''t have to rely on just name to 
derive its meaning: we can go to definition and read the comment. I 
think it''s clear, but I could make it clearer.
> linkprop.c
> 
> - In dladm_init_linkprop() function, it does not seem right to take 
> (flags | DLADM_OPT_PERSIST) as the last argument. Note the possible 
> flags value here is either DLADM_OPT_PROP_MAC_ONLY or 
> DLADM_OPT_PROP_MAC_NAME.
You''re right, I should pass the flags via ''argp''
argument instead.

thanks,
-Artem

Artem Kachitchkine wrote:
> Cathy,
> 
>> 1. Can you think of any non-physical link driver (or even softmac) 
>> could call mac_prop_init()s? I ask this question because it is 
>> impossible for pseudo links (aggr, VNIC, or even softmac) to provide 
>> the instance argument. That number is assigned by the GLDv3 framework.
> 
> This was discussed before and during ARC review and we feel that right
> now mac_prop_init/get will be used by physical drivers only. The need
> for drivers to actively retrieve properties is currently very limited,
> mostly due to some drivers'' design defects/peculiarities. Most
drivers
> should be able to live without these functions, relying on mc_setprop
> callbacks only.
> 
If so, please make it clear that this is only used for physical links.

This leads to another two questions:

a. Do you expect legacy physical drivers to use this interface, and how?

b. Can we remove the second m_instance argument? As I said, it is impossible 
for a driver to specify it if it is different from the instance number of the
dip.
>> 2. Assume the code path 
>> mac_start()->mac_prop_load()->dls_mgmt_linkprop_init()->
>> dlmgmt_upcall_linkprop_init()->dlmgmt_getlink_by_dev() ...
>>
>> Note that dlmgmt_getlink_by_dev() can only be used for physical links, 
>> and even for physical links, the device name is not the same as the 
>> mac name (for example, softmacs). Therefore, when 
>> dlmgmt_getlink_by_dev() is called with mac_name, it will return ENOENT 
>> for softmacs and other non-physical links.
>>
>> The similar problem exists in dld_set_public_prop(). It assumes that 
>> mac name is the same as the device name.
> 
> Yes, I think this also chimes with Meem''s comment about passing
linkid
> instead of name. It would be cleaner. I need to do some research. One
> aspect that bothers me is interaction of dlmgmtd with partially attached
> devices - i.e. the attach(9E)->mac_prop_init->mac_prop_load->...
chain.
>   Do you know of any existing kernel functions to translate macnames
> into devnames and vice versa?
> 
No, there is no existing functions can do this. Especially if it is before 
_attach(), when a mac is not even registered, and mac name is not determined
yet.
>> 3. I assume that you know that mac_prop_init() could fail when a new 
>> network interface is plugged in or you fresh-install a system. I guess 
>> since mac_prop_init() is used to apply the persistent link property 
>> configuration, those failures do not matter. But is a console message 
>> needed here if that fails (the bge_attach() function, line 3091)?
> 
> I personally don''t see the need for console spam here.
There''s no
> messages when ddi_prop_get_int() doesn''t find the property, why
should
> mac_prop_get() be different. I asked Ted to review, maybe he can provide
> his opinion also.
> 
Okay.
>> 4. I don''t see dladm init-linkprop in network/physical service
>> anymore. How link properties like autopush can be applied after your 
>> changes?
> 
> Well, this what this RFE is all about: to remove the need to follow each
> ifconfig plumb with dladm init-linkprop. When a link is plumbed, MAC
> start will trigger dladm_init_linkprop() and all these properties,
> including autopush, will be automatically sent to the kernel. I tested
> autopush, among others - it works.
> 
I understand that this RFE is for this purpose. But I have incorrectly thought 
that datalink autopush must be configured before plumbing.

But note mac_start() will be moved to be part of DL_BIND_REQ processing soon 
(as part of the Clearview softmac fast-path work). In my opinion, mac_start() 
should have been called as the result of DL_BIND_REQ, as only from then on, 
the driver is allowed to transfer data.

So my question changes to: Is it possible to move mac_prop_load() call from 
mac_start() to mac_open()?
> Now, as Sowmini noted, we still need dladm init-linkprop to push
> properties _after plumb_ for wireless drivers. So I''m currently
adding
> an option to initialize wifi properties only, what you''ll see in 
> network/physical is:
> 
>     dladm init-linkprop -w
> 
>> mac.c
>>
>> - It does not look right to call mac_prop_update() for the ENOTSUP 
>> case (line 2952).
> 
> Will fix.
> 
Okay.
>> - mac_get_prop() does not seem to be symmetric to mac_set_prop(). I 
>> guess the reason that DLD_PROP_FLAG_MAC_ONLY only applies to 
>> mac_set_prop() is because in that case, the property is already 
>> initialized in the underlying driver. But the current flag name is 
>> misleading because it seems that this property only needs to be known 
>> by the mac layer. Can we change the name of DLD_PROP_FLAG_MAC_ONLY to 
>> something like DLD_PROP_FLAG_DRIVER_IS_SET?
> 
> I feel DLD_PROP_FLAG_DRIVER_IS_SET is opaque. Naming is subjective,
it''s
> a matter of perception. Luckily, we don''t have to rely on just
name to
> derive its meaning: we can go to definition and read the comment. I 
> think it''s clear, but I could make it clearer.
> 
Okay. I don''t feel strong about it, as long as it can be clearer when
people
read the code.
>> linkprop.c
>>
>> - In dladm_init_linkprop() function, it does not seem right to take 
>> (flags | DLADM_OPT_PERSIST) as the last argument. Note the possible 
>> flags value here is either DLADM_OPT_PROP_MAC_ONLY or 
>> DLADM_OPT_PROP_MAC_NAME.
> 
> You''re right, I should pass the flags via ''argp''
argument instead.
> 
Okay.

Thanks
- Cathy

> Perhaps this should be a key item to identify for drivers during code 
> review -- use of anything other than the mc_setprop()/getprop() API 
> should require justification.  If the problem is a driver defect, then 
> it should be fixable.  (If the problem is a requirement to retrieve the 
> property at start-of-day, during attach, because the hardware needs it 
> to initialize properly... that''s a different case.  Hopefully
there
> aren''t too many of those around.)
Perhaps. This is getting into a gray area, where we''re like
"here''s the
ring, Frodo, but", you know, hopefully being increasingly open (source) 
about all things Solaris would help.

-Artem

> If so, please make it clear that this is only used for physical links.
Okay.
> This leads to another two questions:
> 
> a. Do you expect legacy physical drivers to use this interface, and how?
Nope.
> b. Can we remove the second m_instance argument? As I said, it is
impossible
> for a driver to specify it if it is different from the instance number of
the dip.
During design review, we agreed to do it this way just to be consistent 
with mac_register(). I would just leave it there, why split hairs when 
we can not to.
>> aspect that bothers me is interaction of dlmgmtd with partially
attached
>> devices - i.e. the
attach(9E)->mac_prop_init->mac_prop_load->... chain.
>>   Do you know of any existing kernel functions to translate macnames
>> into devnames and vice versa?
>>
> No, there is no existing functions can do this. Especially if it is before 
> _attach(), when a mac is not even registered, and mac name is not
determined yet.
Exactly. That''s the reason I''m using macname, because at this
early
stage it''s the only thing available for sure. Otherwise we get into a 
deadlock.
> I understand that this RFE is for this purpose. But I have incorrectly
thought
> that datalink autopush must be configured before plumbing.
> 
> But note mac_start() will be moved to be part of DL_BIND_REQ processing
soon
> (as part of the Clearview softmac fast-path work). In my opinion,
mac_start()
> should have been called as the result of DL_BIND_REQ, as only from then on,
> the driver is allowed to transfer data.
> 
> So my question changes to: Is it possible to move mac_prop_load() call from
> mac_start() to mac_open()?
Given that mac_open happens before mac_start, I think yes, this should 
be possible. I''ll give it a try.

-Artem

>> This leads to another two questions:
>>
>> a. Do you expect legacy physical drivers to use this interface, and
how?
> 
> Nope.
> 
I suppose that the dip passed into mac_prop_init() function for legacy drivers 
would be the dip of the physical device, instead of the softmac_dip?
>> b. Can we remove the second m_instance argument? As I said, it is
impossible
>> for a driver to specify it if it is different from the instance number
of the dip.
> 
> During design review, we agreed to do it this way just to be consistent 
> with mac_register(). I would just leave it there, why split hairs when 
> we can not to.
> 
Well, I would think that all callers would just set m_instance to 0.
>>> aspect that bothers me is interaction of dlmgmtd with partially
attached
>>> devices - i.e. the
attach(9E)->mac_prop_init->mac_prop_load->... chain.
>>>   Do you know of any existing kernel functions to translate
macnames
>>> into devnames and vice versa?
>>>
>> No, there is no existing functions can do this. Especially if it is
before
>> _attach(), when a mac is not even registered, and mac name is not
determined yet.
> 
> Exactly. That''s the reason I''m using macname, because at
this early
> stage it''s the only thing available for sure. Otherwise we get
into a
> deadlock.
> 
I think you actually are using the device name, instead of the mac name. 
Assuming a ce device, do you use device name (ce0), or a mac name (softmacxxxx)?
> Given that mac_open happens before mac_start, I think yes, this should 
> be possible. I''ll give it a try.
> 
Thank you!
- Cathy

>>> a. Do you expect legacy physical drivers to use this interface, and
how?
>> Nope.
>>
> I suppose that the dip passed into mac_prop_init() function for legacy
drivers
> would be the dip of the physical device, instead of the softmac_dip?
I''m not sure what you mean. Legacy drivers should not call
mac_prop_init().
> I think you actually are using the device name, instead of the mac name. 
> Assuming a ce device, do you use device name (ce0), or a mac name
(softmacxxxx)?
You''re right, I''m using device names. I''ll fix
variable/field names to
reflect that.

-Artem

Artem Kachitchkine wrote:
>>>> a. Do you expect legacy physical drivers to use this interface,
and how?
>>> Nope.
>>>
>> I suppose that the dip passed into mac_prop_init() function for legacy
drivers
>> would be the dip of the physical device, instead of the softmac_dip?
> 
> I''m not sure what you mean. Legacy drivers should not call
mac_prop_init().
> 
Okay. I think I was just confused. In another word, only GLDv3 physical links 
are supported. Is this correct?
>> I think you actually are using the device name, instead of the mac
name.
>> Assuming a ce device, do you use device name (ce0), or a mac name
(softmacxxxx)?
> 
> You''re right, I''m using device names. I''ll fix
variable/field names to
> reflect that.
> 
If only GLDv3 physical links are supported, mac name for those links are 
actually the same as device names.

Thanks
- Cathy

>> I''m not sure what you mean. Legacy drivers should not call
mac_prop_init().
>>
> Okay. I think I was just confused. In another word, only GLDv3 physical
links
> are supported. Is this correct?
Yes.

-Artem

First I''d like to thank you for the interest in our work and timely 
responses; the Brussels team really appreciates this. I''ve posted the 
latest webrev:

	http://cr.opensolaris.org/~artem/pers/

Summary of change:

- Following a productive discussion with Cathy, I moved 
mac_prop_load/unload invocations from MAC to DLS. Properties are loaded 
when a network device is first opened. They are unloaded lazily when the 
devnet structure goes away (rather than in dls_devnet_close, which would 
be way too frequent). I wasn''t sure about the level of concurrency in 
dls_devnet_open_by_dev(), so I tried to be on the safe side.

- As a result, linkid is now the "unit of currency" for the door
upcall.
We only use macname (aka devname for physical devices) for the 
mac_prop_init/MAC_ONLY case. Property lists are now keyed by macname/vid 
(aka spa). We now handle vlan and aggregation properties more correctly 
than before.

- dladm init-linkprop is back in net-physical, with the -w[ireless] option.

- Incorporated other comments from Sowmini and Cathy.

Old webrev is still there, as well as new-vs-old webrev:

	http://cr.opensolaris.org/~artem/pers-0421/
	http://cr.opensolaris.org/~artem/pers-27vs21/

Cscoped ws is still /net/aja.sfbay/export0/brussels/br-pers/

-Artem

Artem Kachitchkine wrote:
> Testing is still in progress, minor changes might still happen and debug 
> printf''s need to be removed, but overall it is ready for review.
>
> 	http://cr.opensolaris.org/~artem/pers-0421/
>
> Anyone is welcome to provide constructive comments, gentle critique, 
> witty remarks and naughty limericks. Following folks might be especially 
> interested:
>
> Cathy and/or Seb: dlmgmtd, dls
> Eric Cheng and/or Michael Lim: mac, dld, dls
> Ted You: bge
> Sowmini: mac, dld, libdladm, dladm, mdb
>
> I''d appreciate your inputs before next Tuesday, April 29th.
>   
I''m looking at the changes from the perspective of a consumer of the
new
*_MAC_ONLY path for handling mac level properties.

linkprop.c  471

Any reason why you don''t add a flags argument to 
dladm_datalink_id2info() instead
of creating i_dladm_datalink_id2nfo()?

linkprop.c 1656

The new flag, DLADM_OPT_PROP_MAC_ONLY is passed through from the 
application
all the way to  dld_set_public_prop().  Is there a way to have that flag 
attached to the specific
property instead of being decided by the application?

mac.c 101

You might want to add a note that explains the case where the properties 
are handled
by the mac layer and not passed down to the driver.

dld_drv.c 536-543

Can you bypass this for cases when setting properties at the mac layer?

dld_drv.c 558

It looks like DLD_PROP_FLAG_MAC_ONLY needs to also have DLD_PROP_MAC_NAME
set for proper handling (if the first is set but the second is not, err 
is set to EINVAL).  Is this intentional? 
For crossbow,  we''d expect to set the first without the second.


-Mike

Artem Kachitchkine wrote:
> First I''d like to thank you for the interest in our work and
timely
> responses; the Brussels team really appreciates this. I''ve posted
the
> latest webrev:
> 
> 	http://cr.opensolaris.org/~artem/pers/
> 
> Summary of change:
> 
> - Following a productive discussion with Cathy, I moved 
> mac_prop_load/unload invocations from MAC to DLS. Properties are loaded 
> when a network device is first opened. They are unloaded lazily when the 
> devnet structure goes away (rather than in dls_devnet_close, which would 
> be way too frequent). I wasn''t sure about the level of concurrency
in
> dls_devnet_open_by_dev(), so I tried to be on the safe side.
> 
As we discussed offline, calling mac_prop_load() in dls_devnet_open_by_dev() 
is fine, but one thing to note is ddp->dd_vlanid could be 
DATALINK_INVALID_LINKID at this point, if this is a /dev node which is plumbed 
when dlmgmtd is not started yet. At that point, its linkid is not assigned.

Further, as we discussed, adding mac_prop_load() in dls_devnet_open_xxx() code 
path will not be able to initialize the link properties for physical links if 
they are used by some MAC clients (for example, when bge1 is added into an 
aggregation). Alternatively, I am wondering whether it is possible to call 
mac_prop_load/unload() in dls_devnet_set/unset(), so that persistent link 
properties would be populated whenever the link exists.

Thirdly, like I mentioned to you, can we make mac_proplist_t to be based on 
linkid instead of spa and move proplist to be part of the dls module instead 
of the mac module? Doing that, I think we could simplify the code a bit.

Let me know if the above is doable. If not, please also let me know and I will 
continue the review.

Thanks
- Cathy
> - As a result, linkid is now the "unit of currency" for the door
upcall.
> We only use macname (aka devname for physical devices) for the 
> mac_prop_init/MAC_ONLY case. Property lists are now keyed by macname/vid 
> (aka spa). We now handle vlan and aggregation properties more correctly 
> than before.
> 
> - dladm init-linkprop is back in net-physical, with the -w[ireless] option.
> 
> - Incorporated other comments from Sowmini and Cathy.
> 
> Old webrev is still there, as well as new-vs-old webrev:
> 
> 	http://cr.opensolaris.org/~artem/pers-0421/
> 	http://cr.opensolaris.org/~artem/pers-27vs21/
> 
> Cscoped ws is still /net/aja.sfbay/export0/brussels/br-pers/
> 
> -Artem
> 
> _______________________________________________
> networking-discuss mailing list
> networking-discuss at opensolaris.org

Michael Lim wrote:
>>
>> 	http://cr.opensolaris.org/~artem/pers-0421/
>>
> linkprop.c  471
> 
> Any reason why you don''t add a flags argument to 
> dladm_datalink_id2info() instead
> of creating i_dladm_datalink_id2nfo()?
I did this to limit the scope of changes to linkprop.c. There are too 
many consumers of dladm_datalink_id2info().

The rest of the questions, please take a look at the latest webrev I''m 
going to send out shortly, where we moved property handling from MAC to 
DLS, and see if they still apply.

thanks,
-Artem

Please review the updated webrev:

	http://cr.opensolaris.org/~artem/pers/

Consulting further with Cathy, property loading/unloading is now 
happening early during {link, linkid} association, that is in 
dls_devnet_set/unset. An asynchronous taskq is used to work around 
deadlocks, and synchronization barriers are established to ensure proper 
operation.

New file dls_prop.c now contains the bulk of property code. ::dls_prop 
macro provides debugging support for it.

Two flags (MAC_ONLY, MAC_NAME) have been merged into a single flag (NODRV).

Other parts have not changed much.

thanks,
-Artem