[ofw] Patch: Fix using lists on ipoib without locks
Tzachi Dar
tzachid at mellanox.co.il
Mon Nov 3 13:33:36 PST 2008
While looking for the race on the lists, I wanted to make sure that I
know where all the places that this structure is being used. Since the
string p_endpt is used many times in the code I wanted to have a unique
identifier that my editor will be able to find easily.
Thanks
Tzachi
> -----Original Message-----
> From: Alex Estrin [mailto:alex.estrin at qlogic.com]
> Sent: Monday, November 03, 2008 2:37 PM
> To: Tzachi Dar; ofw at lists.openfabrics.org
> Subject: RE: [ofw] Patch: Fix using lists on ipoib without locks
>
>
> Hi Tzachi,
>
> What was the reason behind renaming struct member 'p_endpt'
> to 'p_endpt1' ?
>
> > typedef struct _ipoib_send_desc
> > {
> > NDIS_PACKET *p_pkt;
> > - ipoib_endpt_t *p_endpt;
> > + ipoib_endpt_t *p_endpt1;
>
>
> Thanks,
> Alex.
>
> > -----Original Message-----
> > From: ofw-bounces at lists.openfabrics.org
> > [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Tzachi Dar
> > Sent: Saturday, November 01, 2008 2:11 PM
> > To: Tzachi Dar; ofw at lists.openfabrics.org
> > Subject: RE: [ofw] Patch: Fix using lists on ipoib without locks
> >
> > Applied on 1717/8.
> >
> > Thanks
> > Tzachi
> >
> >
> > ________________________________
> >
> > From: ofw-bounces at lists.openfabrics.org
> > [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Tzachi Dar
> > Sent: Thursday, October 30, 2008 12:52 PM
> > To: ofw at lists.openfabrics.org
> > Subject: [ofw] Patch: Fix using lists on ipoib without locks
> >
> >
> > This is the (hopefully) final version of this patch,
> that was already
> > sent to community in the past.
> >
> > Thanks
> > Tzachi
> >
> > Index: ulp/ipoib/kernel/ipoib_port.c
> >
> > ===================================================================
> > --- ulp/ipoib/kernel/ipoib_port.c (revision 1712)
> > +++ ulp/ipoib/kernel/ipoib_port.c (working copy)
> > @@ -495,8 +495,6 @@
> >
> > inline void ipoib_port_deref(ipoib_port_t * p_port, int type)
> > {
> > - cl_obj_deref( &p_port->obj );
> > -
> > #if DBG
> > cl_atomic_dec( &p_port->ref[type % ref_mask] );
> > IPOIB_PRINT( TRACE_LEVEL_INFORMATION, IPOIB_DBG_OBJ,
> > @@ -504,6 +502,8 @@
> > #else
> > UNREFERENCED_PARAMETER(type);
> > #endif
> > + cl_obj_deref( &p_port->obj );
> > +
> > }
> >
> > /* function returns pointer to payload that is going after IP
> > header.
> > @@ -3954,15 +3954,15 @@
> >
> > p_desc->wr.ds_array = p_desc->local_ds;
> >
> > - p_desc->wr.dgrm.ud.remote_qp = p_desc->p_endpt->qpn;
> > + p_desc->wr.dgrm.ud.remote_qp = p_desc->p_endpt1->qpn;
> > p_desc->wr.dgrm.ud.remote_qkey =
> > p_port->ib_mgr.bcast_rec.qkey;
> > - p_desc->wr.dgrm.ud.h_av = p_desc->p_endpt->h_av;
> > + p_desc->wr.dgrm.ud.h_av = p_desc->p_endpt1->h_av;
> > p_desc->wr.dgrm.ud.pkey_index = p_port->pkey_index;
> > p_desc->wr.dgrm.ud.rsvd = NULL;
> >
> > /* Store context in our reserved area of the packet. */
> > IPOIB_PORT_FROM_PACKET( p_desc->p_pkt ) = p_port;
> > - IPOIB_ENDPT_FROM_PACKET( p_desc->p_pkt ) = p_desc->p_endpt;
> > + IPOIB_ENDPT_FROM_PACKET( p_desc->p_pkt ) = p_desc->p_endpt1;
> > IPOIB_SEND_FROM_PACKET( p_desc->p_pkt ) = p_desc->p_buf;
> >
> > IPOIB_EXIT( IPOIB_DBG_SEND );
> > @@ -3983,8 +3983,8 @@
> > p_desc->p_pkt, status );
> > ipoib_inc_send_stat( p_port->p_adapter, IP_STAT_ERROR, 0 );
> > /* Deref the endpoint. */
> > - if( p_desc->p_endpt )
> > - ipoib_endpt_deref( p_desc->p_endpt );
> > + if( p_desc->p_endpt1 )
> > + ipoib_endpt_deref( p_desc->p_endpt1 );
> >
> > if( p_desc->p_buf )
> > {
> > @@ -4044,7 +4044,7 @@
> > for( i = 0; i < num_packets; i++ )
> > {
> > desc.p_pkt = p_packet_array[i];
> > - desc.p_endpt = NULL;
> > + desc.p_endpt1 = NULL;
> > desc.p_buf = NULL;
> >
> > /* Get the ethernet header so we can find the endpoint. */
> > @@ -4107,7 +4107,7 @@
> > p_eth_hdr->dst.addr[3] = ((unsigned
> char*)&p_ip_hdr->dst_ip)[1];
> > }
> > h_end:
> > - status = __send_mgr_queue( p_port, p_eth_hdr,
> &desc.p_endpt );
> > + status = __send_mgr_queue( p_port, p_eth_hdr,
> > &desc.p_endpt1 );
> > cl_perf_stop( &p_port->p_adapter->perf, SendMgrQueue );
> > if( status == NDIS_STATUS_PENDING )
> > {
> > @@ -4216,7 +4216,7 @@
> >
> > desc.p_pkt = IPOIB_PACKET_FROM_LIST_ITEM(
> > cl_qlist_remove_head( &p_port->send_mgr.pending_list ) );
> > - desc.p_endpt = NULL;
> > + desc.p_endpt1 = NULL;
> > desc.p_buf = NULL;
> >
> > /* Get the ethernet header so we can find the endpoint. */
> > @@ -4231,10 +4231,11 @@
> > }
> >
> > cl_perf_start( GetEndpt );
> > - status = __endpt_mgr_ref( p_port, p_eth_hdr->dst,
> &desc.p_endpt );
> > + status = __endpt_mgr_ref( p_port, p_eth_hdr->dst,
> > &desc.p_endpt1 );
> > cl_perf_stop( &p_port->p_adapter->perf, GetEndpt );
> > if( status == NDIS_STATUS_PENDING )
> > {
> > + CL_ASSERT(desc.p_endpt1 == NULL);
> > cl_qlist_insert_head( &p_port->send_mgr.pending_list,
> > IPOIB_LIST_ITEM_FROM_PACKET( desc.p_pkt ) );
> > break;
> > @@ -4242,6 +4243,7 @@
> > else if( status != NDIS_STATUS_SUCCESS )
> > {
> > ASSERT( status == NDIS_STATUS_NO_ROUTE_TO_DESTINATION );
> > + CL_ASSERT(desc.p_endpt1 == NULL);
> >
> > if( ETH_IS_MULTICAST( p_eth_hdr->dst.addr ) )
> > {
> > @@ -4549,10 +4551,10 @@
> >
> > if( p_port->p_local_endpt )
> > {
> > + cl_fmap_remove_item( &p_port->endpt_mgr.gid_endpts,
> > + &p_port->p_local_endpt->gid_item );
> > cl_qmap_remove_item( &p_port->endpt_mgr.mac_endpts,
> > &p_port->p_local_endpt->mac_item );
> > - cl_fmap_remove_item( &p_port->endpt_mgr.gid_endpts,
> > - &p_port->p_local_endpt->gid_item );
> > cl_qmap_remove_item( &p_port->endpt_mgr.lid_endpts,
> > &p_port->p_local_endpt->lid_item );
> >
> > @@ -4618,8 +4620,10 @@
> > /* Destroy all multicast endpoints now that we have
> released the
> > lock. */
> > while( cl_qlist_count( &mc_list ) )
> > {
> > - cl_obj_destroy( &PARENT_STRUCT(
> > cl_qlist_remove_head( &mc_list ),
> > - ipoib_endpt_t, mac_item.pool_item.list_item )->obj );
> > + cl_list_item_t *p_item;
> > + p_item = cl_qlist_remove_head( &mc_list );
> > + p_endpt = PARENT_STRUCT(p_item, ipoib_endpt_t,
> > mac_item.pool_item.list_item);
> > + cl_obj_destroy( &p_endpt->obj);
> > }
> >
> > IPOIB_EXIT( IPOIB_DBG_ENDPT );
> > @@ -4639,8 +4643,11 @@
> > {
> > IPOIB_ENTER( IPOIB_DBG_ENDPT );
> >
> > + /* This function must be called from the recieve path */
> > + CL_ASSERT(p_port->endpt_rdr > 0);
> > +
> > cl_obj_lock( &p_port->obj );
> > - /* Wait for all readers to complete. */
> > + /* Wait for all readers to complete. */
> > while( p_port->endpt_rdr > 1 )
> > ;
> >
> > @@ -5709,14 +5716,21 @@
> > IPOIB_EXIT( IPOIB_DBG_INIT );
> > return;
> > }
> > - cl_obj_unlock( &p_port->obj );
> > p_port->bc_join_retry_cnt = 0;
> > +
> > + while( p_port->endpt_rdr )
> > + {
> > + cl_obj_unlock( &p_port->obj );
> > + cl_obj_lock( &p_port->obj );
> > + }
> > +
> > if(! p_port->p_local_endpt)
> > {
> > ib_port_info_t port_info;
> > cl_memclr(&port_info, sizeof(port_info));
> > port_info.base_lid = p_port->base_lid;
> > status = __endpt_mgr_add_local( p_port, &port_info );
> > + cl_obj_unlock( &p_port->obj );
> > if( status != IB_SUCCESS )
> > {
> > IPOIB_PRINT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
> > @@ -5725,6 +5739,9 @@
> > goto err;
> > }
> > }
> > +
> > + cl_obj_unlock( &p_port->obj );
> > +
> > status = __endpt_mgr_add_bcast( p_port, p_mcast_rec );
> > if( status != IB_SUCCESS )
> > {
> > @@ -5991,6 +6008,11 @@
> > p_port = (ipoib_port_t*)p_mcast_rec->mcast_context;
> >
> > cl_obj_lock( &p_port->obj );
> > + while( p_port->endpt_rdr )
> > + {
> > + cl_obj_unlock( &p_port->obj );
> > + cl_obj_lock( &p_port->obj );
> > + }
> > if( p_port->state != IB_QPS_RTS )
> > {
> > cl_obj_unlock( &p_port->obj );
> > @@ -6005,10 +6027,10 @@
> > ("Invalid state - Aborting.\n") );
> > return;
> > }
> > - cl_obj_unlock( &p_port->obj );
> >
> > if( p_mcast_rec->status != IB_SUCCESS )
> > {
> > + cl_obj_unlock( &p_port->obj );
> > IPOIB_PRINT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
> > ("Multicast join request failed with status %s.\n",
> > p_port->p_adapter->p_ifc->get_err_str(
> > p_mcast_rec->status )) );
> > @@ -6019,7 +6041,6 @@
> > return;
> > }
> >
> > - cl_obj_lock( &p_port->obj );
> > p_item = cl_fmap_get(
> > &p_port->endpt_mgr.gid_endpts,
> > &p_mcast_rec->p_member_rec->mgid );
> > if( p_item == cl_fmap_end( &p_port->endpt_mgr.gid_endpts ) )
> > @@ -6065,6 +6086,9 @@
> > */
> > if( p_endpt->dlid )
> > {
> > + while( p_port->endpt_rdr )
> > + ;
> > +
> > p_qitem = cl_qmap_insert(
> > &p_port->endpt_mgr.lid_endpts, p_endpt->dlid,
> &p_endpt->lid_item
> > );
> > CL_ASSERT( p_qitem == &p_endpt->lid_item );
> > @@ -6294,6 +6318,12 @@
> > cl_qlist_init( &destroy_mc_list );
> >
> > cl_obj_lock( &p_port->obj );
> > + /* Wait for all readers to finish */
> > + while( p_port->endpt_rdr )
> > + {
> > + cl_obj_unlock( &p_port->obj );
> > + cl_obj_lock( &p_port->obj );
> > + }
> > cnt = 0;
> > p_item = cl_qmap_head( &p_port->endpt_mgr.mac_endpts );
> > while( (p_item != cl_qmap_end(
> > &p_port->endpt_mgr.mac_endpts )) && (cnt < GC_MAX_LEAVE_NUM))
> > @@ -6332,7 +6362,7 @@
> > /* Destroy all multicast endpoints now that we have
> released the
> > lock. */
> > while( cl_qlist_count( &destroy_mc_list ) )
> > {
> > - p_endpt = PARENT_STRUCT( cl_qlist_head( &destroy_mc_list ),
> > + p_endpt = PARENT_STRUCT( cl_qlist_remove_head(
> &destroy_mc_list ),
> > ipoib_endpt_t, mac_item.pool_item.list_item );
> > IPOIB_PRINT( TRACE_LEVEL_INFORMATION, IPOIB_DBG_ENDPT,
> > ("mcast garbage collector: destroying endpoint
> > %02x:%02x:%02x:%02x:%02x:%02x \n",
> > @@ -6342,9 +6372,7 @@
> > p_endpt->mac.addr[3],
> > p_endpt->mac.addr[4],
> > p_endpt->mac.addr[5]) );
> > -
> > - cl_obj_destroy( &PARENT_STRUCT(
> > cl_qlist_remove_head( &destroy_mc_list ),
> > - ipoib_endpt_t, mac_item.pool_item.list_item )->obj );
> > + cl_obj_destroy( &p_endpt->obj );
> > }
> > }
> >
> > Index: ulp/ipoib/kernel/ipoib_port.h
> >
> > ===================================================================
> > --- ulp/ipoib/kernel/ipoib_port.h (revision 1711)
> > +++ ulp/ipoib/kernel/ipoib_port.h (working copy)
> > @@ -378,7 +378,7 @@
> > typedef struct _ipoib_send_desc
> > {
> > NDIS_PACKET *p_pkt;
> > - ipoib_endpt_t *p_endpt;
> > + ipoib_endpt_t *p_endpt1;
> > send_buf_t *p_buf;
> > ib_send_wr_t wr;
> > ipoib_hdr_t pkt_hdr;
> >
> >
> >
>
More information about the ofw
mailing list