Vasu Dev wrote:
> On Wed, 2009-11-25 at 16:53 -0800, Joe Eykholt wrote:
>> My point was that after the link flap occurs, DCBX will restablish PFC
>> and then set enabled, which may not be what the user wanted. Basically,
>> with fcoemon, the user needs to tell fcoemon what's wanted, and not
>> directly use /sys.
>>
>
> I see, I meant only fcoemon will use added enable/disable and for that
> fcoemon changes yet to be done and those changes will determine when to
> use enable/disable from fcoemon, any case user is not suppose to
> directly use sysfs and infact fcoemon will be the only sole user to use
> sysfs for create/destroy also. The fcoeadm will tell fcoemon to create
> and destroy.
>
> Use of netlink could prevent sysfs exposure for enable/disable but that
> would work against WIP to add fcoe container to have fcoe specific
> attributes in FC transport class which is more sysfs centric.
>
>>> So added lport->enabled is similar to lport->link_up but this time to
>>> track user enable/disable state, perhaps can be worked around by
>>> introducing additional states in lport FSM.
>> I mentioned this because I added an enabled flag before along with my rport
>> changes, the goal was to not re-login after fabric_logoff was called
>> while shutting down. Robert suggested I use states instead, and I
>> added the disabled state to cover that.
>>
>
> I see, I'll also try to use disable state here.
>
>>> I'll look into eliminating lport->enabled, perhaps by adding next state
>>> to fc_lport_enter_reset and additional check to not transition lport on
>>> link flap from certain states.
>>>
>>> <snip>
>>>>>> +
>>>>>> + rtnl_lock();
>>>>>> + fcoe = fcoe_hostlist_lookup_port(netdev);
>>>>>> + if (!fcoe) {
>>>>>> + rtnl_unlock();
>>>>>> + rc = -ENODEV;
>>>>>> + goto out_putdev;
>>>>>> + }
>>>>>> + rtnl_unlock();
>>>> You could drop rtnl_lock() before the if-statement.
>>> I'll do that.
>>>
>>>> Actually, the list can't change because fcoe_config_mutex is held,
>>>> so rtnl_lock isn't needed.
>>>>
>>> You are suggesting to add a bug here by removing rtnl lock:-) though
>>> with very very unlikly race. In my understanding the fcoe_config_mutex
>>> is not held on NETDEV_UNREGISTER event processing which could remove
>>> fcoe instance under rtnl_lock.
>> If that's the case, then the fcoe can go away as soon as you drop the
>> rtnl_lock,
>> and you have a bug anyway. NETDEV_UNREGISTER queues a work_struct which
>> gets the fcoe_config_mutex. I verified that fcoe_config_mutex is always
>> held when the list is changed. Let me know if you still think I missed
>> something.
>>
>
> The code snippet below removes fcoe instance only under rtnl_lock w/o
> fcoe_config_mutex held.
>
> case NETDEV_UNREGISTER:
> list_del(&fcoe->list);
> port = lport_priv(fcoe->ctlr.lp);
> fcoe_interface_cleanup(fcoe);
> schedule_work(&port->destroy_work);
>
> So in this case work is scheduled after fcoe is removed which later
> grabs fcoe_config_mutex. You are right it would be a bug to drop
> rtnl_lock sooner in my case. I'll fix that.
I didn't see that before.
Now I see you do need rtnl_lock to protect the list, but can drop it
as soon as you get back from the lookup. The config lock keeps
the fcoe from being freed, so you're OK. Your code is fine.
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
