Re: [Scst-devel] active_cmd_list

[Jain, A. <AJ...@da...>]

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.