<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=iso-8859-1"><meta name=Generator content="Microsoft Word 14 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";}
span.EmailStyle18
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:windowtext;}
span.EmailStyle19
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:100032797;
mso-list-type:hybrid;
mso-list-template-ids:-727051196 67698705 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l0:level1
{mso-level-text:"%1\)";
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level2
{mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level3
{mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level4
{mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level5
{mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level6
{mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level7
{mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level8
{mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level9
{mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1
{mso-list-id:1096170516;
mso-list-type:hybrid;
mso-list-template-ids:1952978164 67698703 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l1:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:.25in;
text-indent:-.25in;}
@list l1:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
margin-left:.75in;
text-indent:-.25in;}
@list l1:level3
{mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level4
{mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level5
{mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level6
{mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level7
{mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level8
{mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1:level9
{mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2
{mso-list-id:1142305636;
mso-list-type:hybrid;
mso-list-template-ids:-2025694854 67698689 67698691 67698693 67698689 67698691 67698693 67698689 67698691 67698693;}
@list l2:level1
{mso-level-number-format:bullet;
mso-level-text:\F0B7;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;
font-family:Symbol;}
@list l2:level2
{mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level3
{mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level4
{mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level5
{mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level6
{mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level7
{mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level8
{mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2:level9
{mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;}
ol
{margin-bottom:0in;}
ul
{margin-bottom:0in;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='color:#1F497D'>Hi Tzachi,<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='color:#1F497D'>You’re right that if the SA query for node records completes before the SA query for path records then __process_query will be called twice with disastrous effects. The fix here is to take an extra reference on query_cnt while the queries are being initiated (for the duration of __ioc_query_sa.) Note that when you decrement this extra reference in __ioc_query_sa, you will need to call __process_query if you released the last reference.<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='color:#1F497D'>The same logic needs to be applied in other functions (__query_iou, __query_ioc_profile, and __query_svc_entries) to prevent the reference count from bouncing, though that’s less likely to occur (the logic that sends all the DM MADs.) Note that some of the error handling looks broken (__ioc_pnp_send_cb of IO_UNIT_INFO timeout is one where it just returns without decrementing the count – it should probably just ‘break’ instead.)<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='color:#1F497D'>The ref counting looks right other than that. The behavior should be straight forward in any case: take a reference before sending out a MAD (whether via ib_query_sa or by explicitly sending a DM MAD.) When that MAD completes, release the reference. You can either use locks to serialize initiating requests and their callbacks, or you can add an extra reference when initiating to prevent the count from bouncing around 0 and minimize serialization. The code attempts to do the latter, but doesn’t account for fast responses.<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='color:#1F497D'>-Fab<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><div><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in'><p class=MsoNormal><b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'>From:</span></b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'> Tzachi Dar [mailto:tzachid@mellanox.co.il] <br><b>Sent:</b> Tuesday, February 08, 2011 9:41 AM<br><b>To:</b> Fab Tillier; ofw@lists.openfabrics.org<br><b>Subject:</b> A problem in the ai_ioc_pnp.c file<o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>Hello Fab,<o:p></o:p></p><p class=MsoNormal>Recently, we got the following assert at __ioc_async_cb: CL_ASSERT( !gp_ioc_pnp->query_cnt ); (==-1)<o:p></o:p></p><p class=MsoNormal>We suspect the problem appeared because of:<o:p></o:p></p><p class=MsoListParagraph style='margin-left:.25in;text-indent:-.25in;mso-list:l1 level1 lfo2'><![if !supportLists]><span style='mso-list:Ignore'>1.<span style='font:7.0pt "Times New Roman"'> </span></span><![endif]>From __ioc_pnp_timer_cb(<o:p></o:p></p><p class=MsoNormal><b><span style='font-size:10.0pt'> /* Pre-charge the ref count so that we don't toggle between 0 and 1. */<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> cl_atomic_inc( &p_mgr->query_cnt );<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> /* Take a reference on the object for the duration of the sweep process. */<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> ref_al_obj( &p_mgr->obj );<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> for( p_item = cl_qlist_head( &p_mgr->obj.obj_list );<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> p_item != cl_qlist_end( &p_mgr->obj.obj_list );<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> p_item = cl_qlist_next( p_item ) )<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> {<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> p_svc = PARENT_STRUCT( PARENT_STRUCT( p_item, al_obj_t, pool_item ),<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> ioc_pnp_svc_t, obj );<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> cl_atomic_inc( &p_mgr->query_cnt );<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> status = __ioc_query_sa( p_svc );<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> if( status != IB_SUCCESS )<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> cl_atomic_dec( &p_mgr->query_cnt );<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> }<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> /* Release the reference we took and see if we're done sweeping. */<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> if( !cl_atomic_dec( &p_mgr->query_cnt ) )<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'> cl_async_proc_queue( gp_async_pnp_mgr, &p_mgr->async_item )<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in;text-indent:-.25in;mso-list:l1 level1 lfo2'><![if !supportLists]><span style='mso-list:Ignore'>2.<span style='font:7.0pt "Times New Roman"'> </span></span><![endif]>Here one assumes that either one of 2 situations is true:<o:p></o:p></p><p class=MsoListParagraph style='text-indent:-.25in;mso-list:l2 level1 lfo4'><![if !supportLists]><span style='font-family:Symbol'><span style='mso-list:Ignore'>·<span style='font:7.0pt "Times New Roman"'> </span></span></span><![endif]>__ioc_query_sa – returns success and later the reference will be removed by the appropriate callback exactly one time<o:p></o:p></p><p class=MsoListParagraph style='text-indent:-.25in;mso-list:l2 level1 lfo4'><![if !supportLists]><span style='font-family:Symbol'><span style='mso-list:Ignore'>·<span style='font:7.0pt "Times New Roman"'> </span></span></span><![endif]>It fails and the reference will be removed immediately<o:p></o:p></p><p class=MsoNormal>Occasionally, this is not the case<o:p></o:p></p><p class=MsoListParagraph style='margin-left:.25in;text-indent:-.25in;mso-list:l1 level1 lfo2'><![if !supportLists]><span style='mso-list:Ignore'>3.<span style='font:7.0pt "Times New Roman"'> </span></span><![endif]>__ioc_query_sa calls twice to ib_query: for Node and Path queries. Thus, 2 callbacks will be invoked thereafter: __node_rec_cb and __path_rec_cb<o:p></o:p></p><p class=MsoListParagraph style='margin-left:.25in;text-indent:-.25in;mso-list:l1 level1 lfo2'><![if !supportLists]><span style='mso-list:Ignore'>4.<span style='font:7.0pt "Times New Roman"'> </span></span><![endif]>Each of these callback may (and will) call to __process_query()<o:p></o:p></p><p class=MsoListParagraph style='margin-left:.25in;text-indent:-.25in;mso-list:l1 level1 lfo2'><![if !supportLists]><span style='mso-list:Ignore'>5.<span style='font:7.0pt "Times New Roman"'> </span></span><![endif]>If process_query will be called twice, that query_cnt will be decreased twice and we are in trouble. <o:p></o:p></p><p class=MsoListParagraph style='margin-left:.25in;text-indent:-.25in;mso-list:l1 level1 lfo2'><![if !supportLists]><span style='mso-list:Ignore'>6.<span style='font:7.0pt "Times New Roman"'> </span></span><![endif]>Theoretically, there is another counter that should avoid this case.<o:p></o:p></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'>From __ioc_query_sa():<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'>……..<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'>cl_atomic_inc( &p_svc->query_cnt );</span></b><b><span style='font-size:10.0pt;font-family:Wingdings'>ß</span></b><b><span style='font-size:10.0pt'>-Raise reference to ensure that __process_query will be called only once<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'>status = ib_query( gh_al, &query, &p_svc->h_node_query );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> if( status != IB_SUCCESS )<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> {<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> cl_atomic_dec( &p_svc->query_cnt );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> deref_al_obj( &p_svc->obj );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> AL_PRINT_EXIT( TRACE_LEVEL_WARNING, AL_DBG_PNP,<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> ("ib_query returned %s\n", ib_get_err_str( status )) );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> return status;<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> }<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'>…………..<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> /* Reference the service for the node record query. */<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> ref_al_obj( &p_svc->obj );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> cl_atomic_inc( &p_svc->query_cnt );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'><o:p> </o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> status = ib_query( gh_al, &query, &p_svc->h_path_query );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> if( status != IB_SUCCESS )<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> {<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> cl_atomic_dec( &p_svc->query_cnt );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> deref_al_obj( &p_svc->obj );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> AL_PRINT_EXIT( TRACE_LEVEL_WARNING, AL_DBG_PNP,<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> ("ib_query returned %s\n", ib_get_err_str( status )) );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> }<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'>From __path_rec_cb and node_rec_cb:<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'><o:p> </o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'>if( !cl_atomic_dec( &p_svc->query_cnt ) ) //should avoid calling __process_query twice<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> {<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> /* The node query has already completed. Process the results. */<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> __process_query( p_svc );<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'> }<o:p></o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'><o:p> </o:p></span></b></p><p class=MsoListParagraph style='margin-left:.25in'><b><span style='font-size:10.0pt'>The issues that we see with the following situation are this:<o:p></o:p></span></b></p><p class=MsoListParagraph style='text-indent:-.25in;mso-list:l0 level1 lfo6'><![if !supportLists]><b><span style='font-size:10.0pt'><span style='mso-list:Ignore'>1)<span style='font:7.0pt "Times New Roman"'> </span></span></span></b><![endif]><span style='font-size:10.0pt'>When __ioc_query_sa is running, the first query will be executed and it’s call back will be called before the second query starts. In this case, the </span>__node_rec_cb will decrease the <span style='font-size:10.0pt'>p_svc->query_cnt and bring it to 0<b>. </b></span>Thus, __process_query will be called twice !<b><span style='font-size:10.0pt'><o:p></o:p></span></b></p><p class=MsoListParagraph style='text-indent:-.25in;mso-list:l0 level1 lfo6'><![if !supportLists]><b><span style='font-size:10.0pt'><span style='mso-list:Ignore'>2)<span style='font:7.0pt "Times New Roman"'> </span></span></span></b><![endif]><span style='font-size:10.0pt'>When looking at the function __process_query we expect it to call cl_atomic_dec( &gp_ioc_pnp->query_cnt)exactly one time. However, if the code executes till the end of the function, than we reach a case statement in which in the case of status == success the reference will not be removed.<b><o:p></o:p></b></span></p><p class=MsoListParagraph style='text-indent:-.25in;mso-list:l0 level1 lfo6'><![if !supportLists]><span style='font-size:10.0pt'><span style='mso-list:Ignore'>3)<span style='font:7.0pt "Times New Roman"'> </span></span></span><![endif]><span style='font-size:10.0pt'>p_svc->query_cnt seems to be like a reference count that is protecting gp_ioc_pnp->query_cnt. However this only works if p_svc->query_cnt was 0 when __ioc_pnp_timer_cb was called and no one has touched it on the entire time. However, we see that p_svc->query_cnt is being touched in many places in the code, and we don’t seem to understand why this assumptions are right.<o:p></o:p></span></p><p class=MsoListParagraph style='text-indent:-.25in;mso-list:l0 level1 lfo6'><![if !supportLists]><span style='font-size:10.0pt'><span style='mso-list:Ignore'>4)<span style='font:7.0pt "Times New Roman"'> </span></span></span><![endif]><span style='font-size:10.0pt'>What is the consequences of calling __process_query more than once (this is an issue that we believe that might currently happen in the code).<o:p></o:p></span></p><p class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>We are still trying to understand the assumptions that this code is based on.<o:p></o:p></p><p class=MsoNormal><b><span style='font-size:10.0pt'><o:p> </o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'>Thanks<o:p></o:p></span></b></p><p class=MsoNormal><b><span style='font-size:10.0pt'>Tzachi & </span>Alexander (XaleX) Naslednikov<o:p></o:p></b></p><p class=MsoNormal><o:p> </o:p></p></div></body></html>