On Thu, 2011-12-01 at 04:17 -0500, Christoph Hellwig wrote:
> > +static void transport_add_tasks_from_cmd(struct se_cmd *cmd)
> > +{
> > + unsigned long flags;
> > + struct se_device *dev = cmd->se_dev;
> > +
> > + spin_lock_irqsave(&dev->execute_task_lock, flags);
> > + __transport_add_tasks_from_cmd(cmd);
> > spin_unlock_irqrestore(&dev->execute_task_lock, flags);
> > }
>
> Given that there is only one caller of this one left I'd remove it an
> opencode the locking there.
>
<nod>
> >
> > @@ -2142,19 +2149,16 @@ static int transport_execute_tasks(struct se_cmd *cmd)
> > if (!add_tasks)
> > goto execute_tasks;
> > /*
> > + * __transport_execute_tasks() -> __transport_add_tasks_from_cmd()
> > + * adds associated se_tasks while holding dev->execute_task_lock
> > + * before I/O dispath to avoid a double spinlock access.
> > */
>
> I don't think this comment adds any value. If at all a
>
> /* expects execute_task_lock to be held */
>
> comment ontop of __transport_execute_tasks would be more useful.
>
...
> > + __transport_execute_tasks(se_dev, cmd);
> > + return 0;
> > }
> > +
> > execute_tasks:
> > + __transport_execute_tasks(se_dev, NULL);
> > return 0;
> > }
>
> Does it still make any sense to run the queue if
> transport_cmd_check_stop return true from the submission context?
>
Not completely sure atm.
> > @@ -2164,7 +2168,7 @@ execute_tasks:
> > *
> > * Called from transport_processing_thread()
> > */
>
> That comment hasn't been true for a while, time to remove it.
>
> > -static int __transport_execute_tasks(struct se_device *dev)
> > +static int __transport_execute_tasks(struct se_device *dev, struct se_cmd *new_cmd)
>
> over 8- charactres line.
>
Fixing up.
> > {
> > int error;
> > struct se_cmd *cmd = NULL;
> > @@ -2176,8 +2180,10 @@ static int __transport_execute_tasks(struct se_device *dev)
> > * set enforce_dev_tcq=0, and dispatch new tasks as soon as
> > * possible.
> > */
> > -check_depth:
> > spin_lock_irq(&dev->execute_task_lock);
> > + if (new_cmd != NULL)
> > + __transport_add_tasks_from_cmd(new_cmd);
> > +
> > if (dev->enforce_dev_tcq) {
> > if (!dev->depth_left) {
> > spin_unlock_irq(&dev->execute_task_lock);
> > @@ -2225,8 +2231,6 @@ check_depth:
> > transport_generic_request_failure(cmd);
> > }
> >
> > - goto check_depth;
> > -
>
> What replacing the looping over multiple tasks? Both the initial task
> submission and the target processing thread might have multiple tasks
> to work on.
>
Hrmmmm, your right here. In that case, I'll have a look at changing
__transport_execute_tasks() to only execute tasks from the same se_cmd.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
