[ofa-general] Re: [PATCH 3/3] Infiniband: don't use task_struct.tgid in make_cm_node()

[Roland Dreier]

 >  	/* get a unique session ID , add thread_id to an upcounter to handle race */
 >  	atomic_inc(&cm_core->node_cnt);
 >  	atomic_inc(&cm_core->session_id);
 > -	cm_node->session_id = (u32)(atomic_read(&cm_core->session_id) + current->tgid);
 > +	cm_node->session_id = (u32)(atomic_read(&cm_core->session_id) +
 > +			task_tgid_nr(current));

Again the change of current->tgid to task_tgid_nr() looks fine as far
as cleaning up the existing code, but again I think we should be able
to get rid of this completely...

First, the use of the tgid seems to be there to protect against a race
like the following where the same value gets used twice:

	atomic_inc()
					atomic_inc()
	atomic_read()
					atomic_read()

but of course it does leave another possibility of an unlucky pair of
tgid values that differ by one and get a collision anyway.  And it
seems like this can be handled more cleanly with atomic_inc_return(),
and not try to use tgid at all.

Second, I don't see anywhere that actually uses the session_id field
after it gets set, so can we just delete it completely?  The patch
below compiles and links fine for me, so is the simplest fix?  (I
don't think hardware is looking at session_id behind our back, is it?)

diff --git a/drivers/infiniband/hw/nes/nes_cm.c b/drivers/infiniband/hw/nes/nes_cm.c
index c1a85f3..31a28e9 100644
--- a/drivers/infiniband/hw/nes/nes_cm.c
+++ b/drivers/infiniband/hw/nes/nes_cm.c
@@ -365,7 +365,6 @@ static void print_core(struct nes_cm_core *core)
 	if (!core)
 		return;
 	nes_debug(NES_DBG_CM, "---------------------------------------------\n");
-	nes_debug(NES_DBG_CM, "Session ID    : %u \n", atomic_read(&core->session_id));
 
 	nes_debug(NES_DBG_CM, "State         : %u \n",  core->state);
 
@@ -1100,8 +1099,6 @@ static struct nes_cm_node *make_cm_node(struct nes_cm_core *cm_core,
 	cm_node->tcp_cntxt.rcv_nxt = 0;
 	/* get a unique session ID , add thread_id to an upcounter to handle race */
 	atomic_inc(&cm_core->node_cnt);
-	atomic_inc(&cm_core->session_id);
-	cm_node->session_id = (u32)(atomic_read(&cm_core->session_id) + current->tgid);
 	cm_node->conn_type = cm_info->conn_type;
 	cm_node->apbvt_set = 0;
 	cm_node->accept_pend = 0;
@@ -1628,9 +1625,7 @@ static struct nes_cm_listener *mini_cm_listen(struct nes_cm_core *cm_core,
 	listener->cm_core = cm_core;
 	listener->nesvnic = nesvnic;
 	atomic_inc(&cm_core->node_cnt);
-	atomic_inc(&cm_core->session_id);
 
-	listener->session_id = (u32)(atomic_read(&cm_core->session_id) + current->tgid);
 	listener->conn_type = cm_info->conn_type;
 	listener->backlog = cm_info->backlog;
 	listener->listener_state = NES_CM_LISTENER_ACTIVE_STATE;
@@ -1943,7 +1938,6 @@ static struct nes_cm_core *nes_cm_alloc_core(void)
 	cm_core->state = NES_CM_STATE_INITED;
 	cm_core->free_tx_pkt_max = NES_CM_DEFAULT_FREE_PKTS;
 
-	atomic_set(&cm_core->session_id, 0);
 	atomic_set(&cm_core->events_posted, 0);
 
 	/* init the packet lists */
diff --git a/drivers/infiniband/hw/nes/nes_cm.h b/drivers/infiniband/hw/nes/nes_cm.h
index 980fb67..7717cb2 100644
--- a/drivers/infiniband/hw/nes/nes_cm.h
+++ b/drivers/infiniband/hw/nes/nes_cm.h
@@ -225,7 +225,6 @@ enum nes_cm_listener_state {
 
 struct nes_cm_listener {
 	struct list_head           list;
-	u64                        session_id;
 	struct nes_cm_core         *cm_core;
 	u8                         loc_mac[ETH_ALEN];
 	nes_addr_t                 loc_addr;
@@ -242,7 +241,6 @@ struct nes_cm_listener {
 
 /* per connection node and node state information */
 struct nes_cm_node {
-	u64                       session_id;
 	u32                       hashkey;
 
 	nes_addr_t                loc_addr, rem_addr;
@@ -327,7 +325,6 @@ struct nes_cm_event {
 
 struct nes_cm_core {
 	enum nes_cm_node_state  state;
-	atomic_t                session_id;
 
 	atomic_t                listen_node_cnt;
 	struct nes_cm_node      listen_list;