DPC vs tasklet

DPC vs tasklet

[Infante, Jon]

Christoph,

I was going to take your advice:
"usage of tasklet itself is questionable, you probably want a kernel-thread"

and redo our driver bottom half discovery handler to use a DPC. I just
wanted to get your opinion of why usage of a tasklet is questionable. Some
of the LINUX documentation I've read states "tasklets are the preferred
mechanism with which to implement your bottom half for a normal hardware
device". I can see, as you stated, being in user context will help with
things like allowing the use of GFP_KERNEL for memory allocations or safe
calling of del_timer_sync(); but its not real clear to me what all the
tradeoffs are and why some people think tasklets are the preferred
mechanism. Can you shed some more light on this or direct me to some URLs
with more info.

Jon Infante
Emulex


-----Original Message-----
From: Christoph Hellwig [mailto:hch@infradead.org]
Sent: Tuesday, May 18, 2004 5:08 AM
To: Smart, James
Cc: 'linux-scsi@vger.kernel.org'
Subject: Re: [(re)Announce] Emulex LightPulse Device Driver


Okay, I took a look at the tasklet handling and have re-arranged it a litte:

 - unused member task_discq_cnt of lpfcHBA_t removed
 - lpfc_discq_tasklet merged into lpfc_discq_post_event and streamlined
 - splice the list in lpfc_tasklet to shorten lock holding (and irq
disabling)
   time
 - use list_for_each_entry to be readable
 - split into subfunctions

I'm still far less than happy with the whole tasklet code, though:

 - lpfc_discq_post_event allocates a structure for every event from irq
   kontext.  Look at qla2xxx how to embedd the lists into the actual
   object to solve this (OTOH qla2xxx is much much nicer in that respect
   anyway as there's only one type of object queued up from the isr to
   the dpc thread)
 - usage of tasklet itself is questionable, you probably want a
kernel-thread
   ala qla2xxx so it's always in usercontext, making your life much easier
   in many places
 - phba->tasklet_running is a mess.  You couldn't care less whether the
   tasklet is running but rather you want to know whether you're called from
   it.


diff -urp lpfcdriver-2.6-8.0.1/lpfc.h lpfcdriver-2.6-8.0.1.hch/lpfc.h
--- lpfcdriver-2.6-8.0.1/lpfc.h	2004-05-16 20:03:37.000000000 +0200
+++ lpfcdriver-2.6-8.0.1.hch/lpfc.h	2004-05-18 13:23:37.491363504 +0200
@@ -399,7 +399,6 @@ typedef struct lpfcHBA {
 	spinlock_t            task_lock;
 	struct tasklet_struct task_run;
 	struct list_head      task_disc;
-	uint16_t              task_discq_cnt;
 	uint16_t	      tasklet_running;
 
 	atomic_t cmnds_in_flight;
diff -urp lpfcdriver-2.6-8.0.1/lpfc_fcp.c
lpfcdriver-2.6-8.0.1.hch/lpfc_fcp.c
--- lpfcdriver-2.6-8.0.1/lpfc_fcp.c	2004-05-16 20:03:37.000000000 +0200
+++ lpfcdriver-2.6-8.0.1.hch/lpfc_fcp.c	2004-05-18 13:55:23.843553920 +0200
@@ -2097,111 +2097,98 @@ lpfc_sleep(lpfcHBA_t * phba, void *wait_
 		return (ETIMEDOUT);
 }
 
-void
-lpfc_discq_tasklet(lpfcHBA_t * phba, LPFC_DISC_EVT_t * evtp)
+int
+lpfc_discq_post_event(lpfcHBA_t * phba, void *arg1, void *arg2, uint32_t
evt)
 {
-	unsigned long flags;
+	LPFC_DISC_EVT_t	*evtp;
+	unsigned long	flags;
+
+	/*
+	 * All Mailbox completions and LPFC_ELS_RING rcv ring events will be
+	 * queued to tasklet for processing
+	 */
+	evtp = kmalloc(sizeof(LPFC_DISC_EVT_t), GFP_ATOMIC);
+	if (unlikely(!evtp))
+		return 0;
+
+	evtp->evt_arg1  = arg1;
+	evtp->evt_arg2  = arg2;
+	evtp->evt       = evt;
 
-	/* Queue the cmnd to the iodone tasklet to be scheduled later */
 	spin_lock_irqsave(&phba->task_lock, flags);
 	list_add_tail(&evtp->evt_listp, &phba->task_disc);
-	phba->task_discq_cnt++;
 	spin_unlock_irqrestore(&phba->task_lock, flags);
+
 	tasklet_schedule(&phba->task_run);
-	return;
+	return 1;
 }
 
-int
-lpfc_discq_post_event(lpfcHBA_t * phba, void *arg1, void *arg2, uint32_t
evt)
+static void
+lpfc_process_evt_mbox(lpfcHBA_t *phba, LPFC_MBOXQ_t *pmb)
+{
+	pmb->mbox_cmpl(phba, pmb);
+}
+
+static void
+lpfc_process_evt_iocb(lpfcHBA_t *phba, LPFC_IOCBQ_t *cmdiocbp,
+		LPFC_RING_MASK_t *func, LPFC_IOCBQ_t *saveq)
 {
-	LPFC_DISC_EVT_t  *evtp;
+	LPFC_IOCBQ_t		*rspiocbp, *tmp;
 
-	/* All Mailbox completions and LPFC_ELS_RING rcv ring events will be
-	 * queued to tasklet for processing
-	 */
-	evtp = (LPFC_DISC_EVT_t *) kmalloc(sizeof(LPFC_DISC_EVT_t),
GFP_ATOMIC);
-	if(evtp == 0) {
-		return (0);
+	if (func) {
+		func->lpfc_sli_rcv_unsol_event(phba,
+				&phba->sli.ring[LPFC_ELS_RING], saveq);
+	} else {
+		cmdiocbp->iocb_cmpl(phba, cmdiocbp, saveq);
 	}
-	evtp->evt_arg1  = arg1;
-	evtp->evt_arg2  = arg2;
-	evtp->evt       = evt;
-	evtp->evt_listp.next = 0;
-	evtp->evt_listp.prev = 0;
-	lpfc_discq_tasklet(phba, evtp);
-	return (1);
+
+	/* free up iocb buffer chain for command just processed */
+	list_for_each_entry_safe(rspiocbp, tmp, &saveq->list, list) {
+		list_del(&rspiocbp->list);
+		mempool_free(rspiocbp, phba->iocb_mem_pool);
+	}
+
+	mempool_free(saveq, phba->iocb_mem_pool);
 }
 
 void
 lpfc_tasklet(unsigned long p)
 {
-	lpfcHBA_t        * phba = (lpfcHBA_t *)p;
-	LPFC_SLI_t       * psli;
-	LPFC_DISC_EVT_t  * evtp;
-	struct list_head * pos, * pos_tmp;
-	struct list_head * cur, * next;
-	LPFC_MBOXQ_t     * pmb;
-	LPFC_IOCBQ_t     * cmdiocbp;
-	LPFC_IOCBQ_t     * rspiocbp;
-	LPFC_IOCBQ_t     * saveq;
-	LPFC_RING_MASK_t * func;
-	unsigned long      flags;
+	lpfcHBA_t	*phba = (lpfcHBA_t *)p;
+	LPFC_DISC_EVT_t	*evtp, *evtp_tmp;
+	unsigned long	flags;
+	LIST_HEAD	(local_task_q);
 
-	psli = &phba->sli;
 	spin_lock_irqsave(&phba->task_lock, flags);
+	list_splice_init(&phba->task_disc, &local_task_q);
+	spin_unlock_irqrestore(&phba->task_lock, flags);
+
 	/* check discovery event list */
-	list_for_each_safe(pos, pos_tmp, &phba->task_disc) {
-		evtp = list_entry(pos, LPFC_DISC_EVT_t, evt_listp);
+	list_for_each_entry_safe(evtp, evtp_tmp, &local_task_q, evt_listp) {
 		list_del(&evtp->evt_listp);
-		phba->task_discq_cnt--;
-		spin_unlock_irqrestore(&phba->task_lock, flags);
 
-		spin_lock_irqsave(&(phba->drvrlock), flags);
-		phba->tasklet_running += 1;
-		switch(evtp->evt) {
+		spin_lock_irqsave(&phba->drvrlock, flags);
+		phba->tasklet_running++;
+		switch (evtp->evt) {
 		case LPFC_EVT_MBOX:
-			pmb = (LPFC_MBOXQ_t *)(evtp->evt_arg1);
-			(pmb->mbox_cmpl) (phba, pmb);
+			lpfc_process_evt_mbox(phba, evtp->evt_arg1);
 			break;
 		case LPFC_EVT_SOL_IOCB:
-			cmdiocbp = (LPFC_IOCBQ_t *)(evtp->evt_arg1);
-			saveq = (LPFC_IOCBQ_t *)(evtp->evt_arg2);
-			(cmdiocbp->iocb_cmpl) (phba, cmdiocbp, saveq);
-			/* Free up iocb buffer chain for command just
-			   processed */
-			list_for_each_safe(cur, next, &saveq->list) {
-				rspiocbp = list_entry(cur, LPFC_IOCBQ_t,
list);
-				list_del(&rspiocbp->list);
-				mempool_free( rspiocbp,
phba->iocb_mem_pool);
-			}
-			mempool_free( saveq, phba->iocb_mem_pool);
+			lpfc_process_evt_iocb(phba, evtp->evt_arg1,
+					NULL, evtp->evt_arg2);
 			break;
 		case LPFC_EVT_UNSOL_IOCB:
-			func = (LPFC_RING_MASK_t *)(evtp->evt_arg1);
-			saveq = (LPFC_IOCBQ_t *)(evtp->evt_arg2);
-			(func->lpfc_sli_rcv_unsol_event) (phba, 
-	 		&psli->ring[LPFC_ELS_RING], saveq);
-			/* Free up iocb buffer chain for command just
-			   processed */
-			list_for_each_safe(cur, next, &saveq->list) {
-				rspiocbp = list_entry(cur, LPFC_IOCBQ_t,
list);
-				list_del(&rspiocbp->list);
-				mempool_free( rspiocbp,
phba->iocb_mem_pool);
-			}
-			mempool_free( saveq, phba->iocb_mem_pool);
+			lpfc_process_evt_iocb(phba, NULL,
+					evtp->evt_arg1, evtp->evt_arg2);
 			break;
 		}
-		phba->tasklet_running -= 1;
-		spin_unlock_irqrestore(&(phba->drvrlock), flags);
+		phba->tasklet_running--;
+		spin_unlock_irqrestore(&phba->drvrlock, flags);
 
-		spin_lock_irqsave(&phba->task_lock, flags);
 		kfree(evtp);
 	}
-	spin_unlock_irqrestore(&phba->task_lock, flags);
-	return;
 }
 
-
 module_init(lpfc_init);
 module_exit(lpfc_exit);
 MODULE_LICENSE("GPL");
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DPC vs tasklet

['Christoph Hellwig']

On Thu, May 20, 2004 at 09:52:25AM -0700, Infante, Jon wrote:
> I was going to take your advice:
> "usage of tasklet itself is questionable, you probably want a kernel-thread"
> 
> and redo our driver bottom half discovery handler to use a DPC. I just
> wanted to get your opinion of why usage of a tasklet is questionable. Some
> of the LINUX documentation I've read states "tasklets are the preferred
> mechanism with which to implement your bottom half for a normal hardware
> device". I can see, as you stated, being in user context will help with
> things like allowing the use of GFP_KERNEL for memory allocations or safe
> calling of del_timer_sync(); but its not real clear to me what all the
> tradeoffs are and why some people think tasklets are the preferred
> mechanism. Can you shed some more light on this or direct me to some URLs
> with more info.

tasklets run at softirq time which means you don't want to do too much
work there, in addition to making locking and memory allocation more
difficult for you.  Given that you're doing lots of processing in there
a kernel thread seems like the better choice.  The kernelthread also
has the advtantage that you can bind it to the cpu the irq happens on
to avoid lots of cacheline bouncing.

In the end you probably want to benchmark the different variants against
each other.

Re: DPC vs tasklet

[Jeff Garzik]

Infante, Jon wrote:
> Christoph,
> 
> I was going to take your advice:
> "usage of tasklet itself is questionable, you probably want a kernel-thread"
> 
> and redo our driver bottom half discovery handler to use a DPC. I just
> wanted to get your opinion of why usage of a tasklet is questionable. Some
> of the LINUX documentation I've read states "tasklets are the preferred
> mechanism with which to implement your bottom half for a normal hardware
> device". I can see, as you stated, being in user context will help with
> things like allowing the use of GFP_KERNEL for memory allocations or safe
> calling of del_timer_sync(); but its not real clear to me what all the
> tradeoffs are and why some people think tasklets are the preferred
> mechanism. Can you shed some more light on this or direct me to some URLs
> with more info.


There are tradeoffs.  Using a kernel thread increases latency, and is 
not always the best choice.

It really depends on how much "work" you need to do in the interrupt 
handling and bottom-half phases.

If we are talking per-irq or per-IO, kernel thread is a poor choice.

	Jeff

Re: DPC vs tasklet

[Bryan Henderson]

> Some
> of the LINUX documentation I've read states "tasklets are the preferred
> mechanism with which to implement your bottom half for a normal hardware
> device".

In all the great answers about why other things are better than tasklets, 
the basic question of why this documentation doesn't agree isn't 
addressed.  Let me try:  I believe the context of this statement is a 
comparison to the more traditional Unix method of implementing the bottom 
half in the interrupt handler.  The statement is saying that a tasklet is 
normally preferred to putting the same processing right in the interrupt 
handler.

But in at least some cases, a kernel thread is better than either of 
those.

Re: DPC vs tasklet

[Matthew Wilcox]

On Thu, May 20, 2004 at 05:36:27PM -0700, Bryan Henderson wrote:
> > Some
> > of the LINUX documentation I've read states "tasklets are the preferred
> > mechanism with which to implement your bottom half for a normal hardware
> > device".
> 
> In all the great answers about why other things are better than tasklets, 
> the basic question of why this documentation doesn't agree isn't 
> addressed.  Let me try:  I believe the context of this statement is a 
> comparison to the more traditional Unix method of implementing the bottom 
> half in the interrupt handler.  The statement is saying that a tasklet is 
> normally preferred to putting the same processing right in the interrupt 
> handler.

Actually, I think the quote in context probably refers to choosing
between using an old-style (now extinct) bottom half, a tasklet or
a softirq for doing the work that can't be done in the hardirq context.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: DPC vs tasklet

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

On Fri, 2004-05-21 at 02:36, Bryan Henderson wrote:
> > Some
> > of the LINUX documentation I've read states "tasklets are the preferred
> > mechanism with which to implement your bottom half for a normal hardware
> > device".
> 
> In all the great answers about why other things are better than tasklets, 
> the basic question of why this documentation doesn't agree isn't 
> addressed.  Let me try:  I believe the context of this statement is a 
> comparison to the more traditional Unix method of implementing the bottom 
> half in the interrupt handler.  The statement is saying that a tasklet is 
> normally preferred to putting the same processing right in the interrupt 
> handler.
> 
> But in at least some cases, a kernel thread is better than either of 
> those.

yup. And the scsi driver case is special because the SCSI layer
implements the tasklet/softirq *for you* already, so instead of each
driver having to do it's own, just use the scsi one (by completing
directly in the ISR, which queues for the generic scsi softirq)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: DPC vs tasklet

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On Thu, 2004-05-20 at 18:52, Infante, Jon wrote:
> Christoph,
> 
> I was going to take your advice:
> "usage of tasklet itself is questionable, you probably want a kernel-thread"

well there's a few angles here:
1) For IO completion (eg the common, fastpath case), you don't want to
use a tasklet OR kernel-thread, just do the completion right there and
then in the ISR, since the scsi layer completion handler you call will
queue it for real work, the final completion, by the scsi softirq
already.

2) For management work, I can see the point of doing it in a
kernel-thread, since it's 1) rare (eg slowpath) 2) complex 3) code that
might want long-ish delays or allocate memory, neither of which you want
in irq context.




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: DPC vs tasklet

[Jeff Garzik]

Arjan van de Ven wrote:
> well there's a few angles here:
> 1) For IO completion (eg the common, fastpath case), you don't want to
> use a tasklet OR kernel-thread, just do the completion right there and
> then in the ISR, since the scsi layer completion handler you call will
> queue it for real work, the final completion, by the scsi softirq
> already.

Agreed, presuming that the IO completion is fairly cheap.


> 2) For management work, I can see the point of doing it in a
> kernel-thread, since it's 1) rare (eg slowpath) 2) complex 3) code that
> might want long-ish delays or allocate memory, neither of which you want
> in irq context.

Agreed.

	Jeff

RE: DPC vs tasklet

[Infante, Jon]

FC discovery is a rare occurance and potentially pretty complex. Based on
the input from everyone I feel much more confident about making it a DPC.
Thanks.
Jon

-----Original Message-----
From: Jeff Garzik [mailto:jgarzik@pobox.com]
Sent: Thursday, May 20, 2004 10:58 AM
To: arjanv@redhat.com
Cc: Infante, Jon; 'Christoph Hellwig'; Smart, James;
'linux-scsi@vger.kernel.org'
Subject: Re: DPC vs tasklet


Arjan van de Ven wrote:
> well there's a few angles here:
> 1) For IO completion (eg the common, fastpath case), you don't want to
> use a tasklet OR kernel-thread, just do the completion right there and
> then in the ISR, since the scsi layer completion handler you call will
> queue it for real work, the final completion, by the scsi softirq
> already.

Agreed, presuming that the IO completion is fairly cheap.


> 2) For management work, I can see the point of doing it in a
> kernel-thread, since it's 1) rare (eg slowpath) 2) complex 3) code that
> might want long-ish delays or allocate memory, neither of which you want
> in irq context.

Agreed.

	Jeff