From: Jain, A. <AJ...@da...> - 2009-07-29 22:39:27
|
Hi Vlad, > -----Original Message----- > From: Vladislav Bolkhovitin [mailto:vs...@vl...] > Sent: Wednesday, July 29, 2009 3:04 PM > To: Jain, Arvind > Cc: scs...@li... > Subject: Re: [Scst-devel] active_cmd_list > > Hi, > > Jain, Arvind, on 07/29/2009 09:38 AM wrote: > > Hi Vlad, > > > > > > > > I have a question on the scst_targ.c code, specifically regarding the > > active_cmd_list. > > > > > > > > I noticed that at most of places in the code, you do a list_add() if > > queue_type == SCST_CMD_QUEUE_HEAD_OF_QUEUE, else you do list_add_tail() > > > > > > > > > > > > Code Fragment: > > > > > > > > scst_targ.c @327, @1066 > > > > > > > > --- Start Copy --- > > > > case SCST_CONTEXT_THREAD: > > > > spin_lock_irqsave(&cmd->cmd_lists->cmd_list_lock, flags); > > > > TRACE_DBG("Adding cmd %p to active cmd list", cmd); > > > > if (unlikely(cmd->queue_type == > SCST_CMD_QUEUE_HEAD_OF_QUEUE)) > > > > list_add(&cmd->cmd_list_entry, > > > > &cmd->cmd_lists->active_cmd_list); > > > > else > > > > list_add_tail(&cmd->cmd_list_entry, > > > > &cmd->cmd_lists->active_cmd_list); > > > > wake_up(&cmd->cmd_lists->cmd_list_waitQ); > > > > spin_unlock_irqrestore(&cmd->cmd_lists->cmd_list_lock, > flags); > > > > break; > > > > } > > > > --- End Copy --- > > > > > > > > However at @3550, you do a list_add() unconditionally. > > > > > > > > Code Fragment: > > > > Line @3532 > > > > > > > > --- start copy --- > > > > switch (cmd->state) { > > > > case SCST_CMD_STATE_PRE_PARSE: > > > > case SCST_CMD_STATE_DEV_PARSE: > > > > case SCST_CMD_STATE_PREPARE_SPACE: > > > > case SCST_CMD_STATE_RDY_TO_XFER: > > > > case SCST_CMD_STATE_TGT_PRE_EXEC: > > > > case SCST_CMD_STATE_SEND_FOR_EXEC: > > > > case SCST_CMD_STATE_LOCAL_EXEC: > > > > case SCST_CMD_STATE_REAL_EXEC: > > > > case SCST_CMD_STATE_PRE_DEV_DONE: > > > > case SCST_CMD_STATE_MODE_SELECT_CHECKS: > > > > case SCST_CMD_STATE_DEV_DONE: > > > > case SCST_CMD_STATE_PRE_XMIT_RESP: > > > > case SCST_CMD_STATE_XMIT_RESP: > > > > case SCST_CMD_STATE_FINISHED: > > > > case SCST_CMD_STATE_FINISHED_INTERNAL: > > > > TRACE_DBG("Adding cmd %p to head of active cmd list", > > > > cmd); > > > > list_add(&cmd->cmd_list_entry, > > > > &cmd->cmd_lists->active_cmd_list) > > > > --- end copy --- > > > > > > > > > > > > Could you please let me know what is the reason for doing unconditional > > list_add() @3550? > > If you look a bit above you will see: > > if (res == SCST_CMD_STATE_RES_NEED_THREAD) > > I.e., the case you quoted is triggered if somewhere during processing on > the tasklet context is was found that a thread context is needed to > continue. In this case the corresponding procedure returns > SCST_CMD_STATE_RES_NEED_THREAD and then the command rescheduled to one > of the SCST threads to continue on the same the command's state. This > command already reached its turn in the queue for processing, so it > would be unfair for it to put it in the end of the queue to wait again. > > This SCST_CMD_STATE_RES_NEED_THREAD case is pretty marginal and > triggered very rarely. I've just checked that by changing TRACE_DBG() > above to TRACE_DBG_SPECIAL() with qla2x00t and iscsi-scst with all SCST > dev handlers. The result was as expected: this case was triggered only > on the SGV cache misses with qla2x00t. > > In other words, it can't massively affect commands execution order. > OK, I will look into our code and see why we are hitting this condition so frequently. > > I am asking this question because we had a performance issue running > > scst on a SMP machine because commands were getting out of order for > > simple tagged commands. > > > > > > > > I replaced the code @3550 as follows: > > > > if (unlikely(cmd->queue_type == SCST_CMD_QUEUE_HEAD_OF_QUEUE)) > > > > list_add(&cmd->cmd_list_entry, > > > > &cmd->cmd_lists->active_cmd_list); > > > > else > > > > list_add_tail(&cmd->cmd_list_entry, > > > > &cmd->cmd_lists->active_cmd_list); > > > > > > > > After this change, the performance issue went away as the commands got > > added at the tail and got removed from the head for simple tagged > > commands, keeping the commands in the same order as they came from the > host. > > > > > > > > I will appreciate your insight on this issue. Do you concur with my fix? > > What target and dev handler are you using? In what configuration? > We are using QLogic target and a custom dev handler. We are using Dual-Core AMD Opteron(tm) Processor 8214, Qlogic FC HBA, connected to a FC disk array on the backend. Thanks for all the info. Arvind. |