Roland Dreier wrote: > By the way: > > > + struct ib_sa_sm_ah *sm_ah; > > + > > + spin_lock_irqsave(&port->ah_lock, flags); > > + sm_ah = port->sm_ah; > > + port->sm_ah = NULL; > > + spin_unlock_irqrestore(&port->ah_lock, flags); > > + > > + if (sm_ah) > > + kref_put(&sm_ah->ref, free_sm_ah); > > Is there some reason why this can't be simpler like: > > spin_lock_irqsave(&port->ah_lock, flags); > if (port->sm_ah) > kref_put(&port->sm_ah->ref, free_sm_ah); > port->sm_ah = NULL; > spin_unlock_irqrestore(&port->ah_lock, flags); > What happens if this happens
# | CPU-0 | CPU-1 | | 1 | if (port->sm_ah) | | kref_put(&port->sm_ah->ref, free_sm_ah); | --+-----------------------------------------------------+----------------------- 2 | | alloc_mad() --+-----------------------------------------------------+----------------------- 3 | port->sm_ah = NULL; | As I see it, process on CPU-1 gets a garbage sm_ah Do you agree? > I guess the same cleanup applies to update_sm_ah(), except after your > patch I don't see any way that update_sm_ah() could be called with sm_ah > anything but NULL, so we could drop the old_ah stuff completely there. I agree. The cleanup code can be completely removed. > > - R. > _______________________________________________ > general mailing list > [email protected] > http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general > > To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general > _______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
