Hi Jon, Sorry for replying the mail with outlook client, and making its format messed.
As we now use kref_get_if_not_zero() instead of kref_get() in tipc_node_find(), the risk you said below doesn't exist. The reason why I suggest we move the deletion of node from two lists from tipc_node_delete() to tipc_node_kref_release() is mainly to avoid double deletion of the same node from the two lists if tipc_node_delete() is wrongly called two times for the same node. Additionally, I also prefer use kref_get_if_not_zero() too as the cost of calling synchronize_rcu() is too expensive, and it cannot be called in non-atomic context. Regards, Ying > -----Original Message----- > From: Xue, Ying [mailto:[email protected]] > Sent: Tuesday, 16 February, 2016 08:42 > To: Jon Maloy; [email protected]; Parthasarathy > Bhuvaragan; Richard Alpe; [email protected] > Cc: [email protected] > Subject: RE: [PATCH net-next 1/1] tipc: eliminate risk of finding > to-be-deleted node instance > > @@ -395,21 +396,19 @@ static void tipc_node_delete(struct tipc_node *node) { > list_del_rcu(&node->list); > hlist_del_rcu(&node->hash); > > It's better to move two lines of code above to > tipc_node_kref_release(), ensuring we remove the node from the two lists only > when node is released. I beg to disagree. This was one of the points with this change; to remove the node from the lookup table as early as possible, to reduce the risk that node_find() finds it after it is marked for deletion. Actually, if I had put a synchronize_rcu() after those two lines I believe we would have been completely safe, but I prefer Jason's suggestion, to use kref_put_if_not_zero(). The risk that this function will ever return zero (false) should now be minimal. Besides, I simply think this is more logical, as I stated in the commit log. Regards ///jon > > Regards, > Ying > > - kfree(node->bc_entry.link); > - kfree_rcu(node, rcu); > + tipc_node_put(node); > + if (del_timer(&node->timer)) > + tipc_node_put(node); > } > > void tipc_node_stop(struct net *net) > { > - struct tipc_net *tn = net_generic(net, tipc_net_id); > + struct tipc_net *tn = tipc_net(net); > struct tipc_node *node, *t_node; > > spin_lock_bh(&tn->node_list_lock); > - list_for_each_entry_safe(node, t_node, &tn->node_list, list) { > - if (del_timer(&node->timer)) > - tipc_node_put(node); > - tipc_node_put(node); > - } > + list_for_each_entry_safe(node, t_node, &tn->node_list, list) > + tipc_node_delete(node); > spin_unlock_bh(&tn->node_list_lock); > } > > -- > 1.9.1 ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140 _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
