thanks for the explanation, thiru.
I agree with you that the complete solution will take a lot more work.
I will likely tackle this as part of 6791335 as they are somewhat
related.
eric
On Sun, Jan 18, 2009 at 06:37:35PM -0800, Thirumalai Srinivasan wrote:
>
> Eric,
>
> Iam assuming you mean dls_devnet_unset() will fail the unset if there are
> temporary references i.e. the dd_tref is non-zero or if dd_prop_taskid is
> non-null. This is easy to implement, the only issue is the one Cathy pointed
> For example a script that runs a delete vnic would fail occasionally if it
> coincided with somone running a dladm show command. If this failure is
> acceptable, then there is perhaps no need for even keeping track of a
> separate
> field for temporary references.
>
> (cfgadm failure is perhaps similar, but may not be such a call generator.)
>
>
> >Cathy Zhou wrote:
> >>Can you elaborate what would be the locking issues for aggr/vnic if we
> >>wait in dls_devnet_unset?
> >>
>
>
> If you wait for the dd_tref to drop to zero, while holding the perimeter
> there is the risk of deadlock. For eg. the following sequence in
> drv_ioc_prop_common() would do the opposite order
>
> if ((err = dls_devnet_hold_tmp(linkid, &dlh)) != 0)
> goto done;
>
> if ((err = mac_perim_enter_by_macname(dls_devnet_mac(dlh),
> &mph)) != 0) {
> goto done;
> }
>
>
> The other alternative would need lot more work and code reorganizion. We
> need to
> separate the disable and destroy part all through, in vanity naming,
> softmac, and mac, so that all disables are done first. Then we can undo the
> disables if any of them fail. Only if all disables succeed, should we do the
> destroys. Destroys must never fail.
>
> Thirumalai
>
>
>
> Eric Cheng wrote:
> >I think something like this could work:
> >
> >int
> >dls_devnet_destroy(mac_handle_t mh, datalink_id_t *idp)
> >{
> > int err;
> > mac_perim_handle_t mph;
> >
> > mac_perim_enter_by_mh(mh, &mph);
> >
> > *idp = DATALINK_INVALID_LINKID;
> > err = dls_devnet_unset(mac_name(mh), idp, B_FALSE);
> > if (err != 0) {
> > mac_perim_exit(mph);
> > return (err);
> > }
> > err = dls_link_rele_by_name(mac_name(mh));
> > ASSERT(err == 0);
> > mac_perim_exit(mph);
> > return (err);
> >}
> >
> >static int
> >dls_devnet_unset(const char *macname, datalink_id_t *id, boolean_t wait)
> >{
> > dls_devnet_t *ddp;
> > int err;
> > mod_hash_val_t val;
> >
> > rw_enter(&i_dls_devnet_lock, RW_WRITER);
> > if ((err = mod_hash_find(i_dls_devnet_hash,
> > (mod_hash_key_t)macname, (mod_hash_val_t *)&ddp)) != 0) {
> > ASSERT(err == MH_ERR_NOTFOUND);
> > rw_exit(&i_dls_devnet_lock);
> > return (ENOENT);
> > }
> >
> > if ((err = dls_link_is_busy(macname)) != 0) {
> > rw_exit(&i_dls_devnet_lock);
> > return (err);
> > }
> >
> > mutex_enter(&ddp->dd_mutex);
> > ....
> >}
> >
> >int
> >dls_link_is_busy(const char *name)
> >{
> > dls_link_t *dlp;
> >
> > if (mod_hash_find(i_dls_link_hash, (mod_hash_key_t)name,
> > (mod_hash_val_t *)&dlp) != 0)
> > return (ENOENT);
> >
> > ASSERT(MAC_PERIM_HELD(dlp->dl_mh));
> >
> > /*
> > * Must fail detach if mac client is busy.
> > */
> > ASSERT(dlp->dl_ref > 0 && dlp->dl_mch != NULL);
> > if (mac_link_has_flows(dlp->dl_mch))
> > return (ENOTEMPTY);
> >
> > return (0);
> >}
> >
> >
> >notes:
> >-the DD_CONDEMNED flag has the effect of disabling the dls_devnet_t
> > and ensures that subsequent calls to dls_devnet_hold_tmp() would
> > fail (indirectly called by mac_link_flow_add())
> >
> >-we do not wait in dls_devnet_unset() because this could deadlock
> > if someone is holding a tmp reference. softmac_destroy() already
> > does this so it should be ok to change aggr/vnic to behave the
> > same way.
> >
> >let me know if this makes sense.
> >
> >eric
> >
> >On Fri, Jan 16, 2009 at 03:17:34PM -0800, Cathy Zhou wrote:
> >
> >>On 2009?01?16? 14:34, Sebastien Roy wrote:
> >>
> >>>On Fri, 2009-01-16 at 13:33 -0800, Cathy Zhou wrote:
> >>>
> >>>>On 2009?01?16? 13:16, Sebastien Roy wrote:
> >>>>
> >>>>>Hi Crossbow team,
> >>>>>
> >>>>>I have a question on the modifications made to dls_devnet_destroy():
> >>>>>
> >>>>>int
> >>>>>dls_devnet_destroy(mac_handle_t mh, datalink_id_t *idp, boolean_t wait)
> >>>>>{
> >>>>> int err;
> >>>>> mac_perim_handle_t mph;
> >>>>>
> >>>>> *idp = DATALINK_INVALID_LINKID;
> >>>>> err = dls_devnet_unset(mac_name(mh), idp, wait);
> >>>>> if (err != 0 && err != ENOENT)
> >>>>> return (err);
> >>>>>
> >>>>> mac_perim_enter_by_mh(mh, &mph);
> >>>>> err = dls_link_rele_by_name(mac_name(mh));
> >>>>> mac_perim_exit(mph);
> >>>>>
> >>>>> if (err != 0)
> >>>>> (void) dls_devnet_set(mac_name(mh), *idp, NULL);
> >>>>> return (err);
> >>>>>}
> >>>>>
> >>>>>Once the call to dls_devnet_unset() has succeeded, is it possible for
> >>>>>dls_link_rele_by_name() to fail? I ask because the subsequent call to
> >>>>>dls_devnet_set() upon dls_link_rele_by_name() failure is problematic.
> >>>>>It this dls_devnet_set() call fails, the system is rendered hosed until
> >>>>>rebooted.
> >>>>>
> >>>>>
> >>>>It looks possible. As long as there are still subflows added on this
> >>>>link, it would fail.
> >>>>
> >>>Is there a way to fix this? There has to be some way to freeze the
> >>>state of things once it's been verified that teardown is safe (sort of
> >>>like what mac_disable() was intended to do). That way, one can assert
> >>>that the various steps involved in teardown cannot fail.
> >>>
> >>>
> >>Yes. I believe it is possible. Can we add a dls_devnet_disable()
> >>function, which checks whether we can destroy dls_devnet_t, dls_link_t
> >>(or even mac_impl_t?) and set the disabled flag on each of them. The
> >>disabled flag will be used to prevent further usage of those structures.
> >>
> >>Thanks
> >>- Cathy
> >>_______________________________________________
> >>crossbow-discuss mailing list
> >>crossbow-discuss at opensolaris.org
> >>http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss
> >>
> >_______________________________________________
> >crossbow-discuss mailing list
> >crossbow-discuss at opensolaris.org
> >http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss
> >
>
