RE: [PATCH] aacraid: Add likely() and unlikely()

RE: [PATCH] aacraid: Add likely() and unlikely()

[Allexio Ju]

On Thu, 2007-03-22 at 2007 2:24 AM, Arjan van de Ven wrote:
> (I assume you're aware that likely/unlikely should only be
> used for 99:1 or higher ratios, this one looks correct for sure)
Could you share details of reasons why those macros should be used in the way?
I thought those macros simply tell compiler to layout code in such a
way that minimizes unnecessary jumps.

Thank you,

Allexio

> On Wed, 2007-03-21 at 15:43 -0400, Salyzyn, Mark wrote:
> > Add some likely() and unlikely() compiler hints in some of
> the aacraid
> > hardware interface layers. There should be no operational
> side effects
> > resulting from this patch and the changes should be mostly
> benign on
> > x86 platforms.
> >
> > ObligatoryDisclaimer: Please accept my condolences
> regarding Outlook's
> > handling of patches attachments.
> >
> > This attached patch is against current scsi-misc-2.6
>
> -               if((fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
> +               if(unlikely(fibptr->flags &
> + FIB_CONTEXT_FLAG_TIMED_OUT)){
>                         return -ETIMEDOUT;
>                 } else {
>                         return 0;
>
> while you're at it, please remove the extra {}'s as well
>
> (I assume you're aware that likely/unlikely should only be
> used for 99:1 or higher ratios, this one looks correct for sure)

RE: [PATCH] aacraid: Add likely() and unlikely()

[Arjan van de Ven]

On Tue, 2007-03-27 at 09:17 -0700, Allexio Ju wrote:
> On Thu, 2007-03-22 at 2007 2:24 AM, Arjan van de Ven wrote:
> > (I assume you're aware that likely/unlikely should only be
> > used for 99:1 or higher ratios, this one looks correct for sure)
> Could you share details of reasons why those macros should be used in the way?
> I thought those macros simply tell compiler to layout code in such a
> way that minimizes unnecessary jumps.


it's more than that. it generally also tells the processor what the
branch will be, at which point most processors disable their own branch
prediction logic. Trying to hand-layout code is almost always a
mistake... don't do that. GCC also is quite good at recognizing certain
patterns to keep the code flow working. Trying to override that only
hurts...
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [PATCH] aacraid: Add likely() and unlikely()

[Allexio Ju]

On 3/28/07, Arjan van de Ven <arjan@infradead.org> wrote:
> On Tue, 2007-03-27 at 09:17 -0700, Allexio Ju wrote:
> > I thought those macros simply tell compiler to layout code in such a
> > way that minimizes unnecessary jumps.
> it's more than that. it generally also tells the processor what the
> branch will be, at which point most processors disable their own branch
> prediction logic. Trying to hand-layout code is almost always a
> mistake... don't do that. GCC also is quite good at recognizing certain
> patterns to keep the code flow working. Trying to override that only
> hurts...
I see... thanks for clarification.

allexio

Re: [PATCH] aacraid: Add likely() and unlikely()

[Arjan van de Ven]

On Wed, 2007-03-21 at 15:43 -0400, Salyzyn, Mark wrote:
> Add some likely() and unlikely() compiler hints in some of the aacraid
> hardware interface layers. There should be no operational side effects
> resulting from this patch and the changes should be mostly benign on x86
> platforms.
> 
> ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
> handling of patches attachments.
> 
> This attached patch is against current scsi-misc-2.6

-               if((fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
+               if(unlikely(fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
                        return -ETIMEDOUT;
                } else {
                        return 0;

while you're at it, please remove the extra {}'s as well

(I assume you're aware that likely/unlikely should only be used for 99:1
or higher ratios, this one looks correct for sure)

Also I suggest you don't add about half of these; they appear init time
only code, at which point adding likely/unlikely is just code noise that
clutters the driver...
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

[PATCH] aacraid: Add likely() and unlikely()

[Salyzyn, Mark]

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

Add some likely() and unlikely() compiler hints in some of the aacraid
hardware interface layers. There should be no operational side effects
resulting from this patch and the changes should be mostly benign on x86
platforms.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patches attachments.

This attached patch is against current scsi-misc-2.6

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

---

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_likely_unlikely.patch --]
[-- Type: application/octet-stream, Size: 8177 bytes --]

diff -ru a//drivers/scsi/aacraid/commsup.c b//drivers/scsi/aacraid/commsup.c
--- a//drivers/scsi/aacraid/commsup.c	2007-03-21 15:15:59.470057770 -0400
+++ b//drivers/scsi/aacraid/commsup.c	2007-03-21 15:32:21.779160958 -0400
@@ -519,7 +519,7 @@
 		spin_unlock_irqrestore(&fibptr->event_lock, flags);
 		BUG_ON(fibptr->done == 0);
 			
-		if((fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
+		if(unlikely(fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
 			return -ETIMEDOUT;
 		} else {
 			return 0;
diff -ru a//drivers/scsi/aacraid/rx.c b//drivers/scsi/aacraid/rx.c
--- a//drivers/scsi/aacraid/rx.c	2007-03-21 15:15:59.472057518 -0400
+++ b//drivers/scsi/aacraid/rx.c	2007-03-21 15:32:21.780160831 -0400
@@ -5,7 +5,7 @@
  * based on the old aacraid driver that is..
  * Adaptec aacraid device driver for Linux.
  *
- * Copyright (c) 2000 Adaptec, Inc. (aacraid@adaptec.com)
+ * Copyright (c) 2000-2007 Adaptec, Inc. (aacraid@adaptec.com)
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -57,25 +57,25 @@
 	 *	been enabled.
 	 *	Check to see if this is our interrupt.  If it isn't just return
 	 */
-	if (intstat & ~(dev->OIMR)) {
+	if (likely(intstat & ~(dev->OIMR))) {
 		bellbits = rx_readl(dev, OutboundDoorbellReg);
-		if (bellbits & DoorBellPrintfReady) {
+		if (unlikely(bellbits & DoorBellPrintfReady)) {
 			aac_printf(dev, readl (&dev->IndexRegs->Mailbox[5]));
 			rx_writel(dev, MUnit.ODR,DoorBellPrintfReady);
 			rx_writel(dev, InboundDoorbellReg,DoorBellPrintfDone);
 		}
-		else if (bellbits & DoorBellAdapterNormCmdReady) {
+		else if (unlikely(bellbits & DoorBellAdapterNormCmdReady)) {
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormCmdReady);
 			aac_command_normal(&dev->queues->queue[HostNormCmdQueue]);
 		}
-		else if (bellbits & DoorBellAdapterNormRespReady) {
+		else if (likely(bellbits & DoorBellAdapterNormRespReady)) {
 			rx_writel(dev, MUnit.ODR,DoorBellAdapterNormRespReady);
 			aac_response_normal(&dev->queues->queue[HostNormRespQueue]);
 		}
-		else if (bellbits & DoorBellAdapterNormCmdNotFull) {
+		else if (unlikely(bellbits & DoorBellAdapterNormCmdNotFull)) {
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormCmdNotFull);
 		}
-		else if (bellbits & DoorBellAdapterNormRespNotFull) {
+		else if (unlikely(bellbits & DoorBellAdapterNormRespNotFull)) {
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormCmdNotFull);
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormRespNotFull);
 		}
@@ -88,11 +88,11 @@
 {
 	struct aac_dev *dev = dev_id;
 	u32 Index = rx_readl(dev, MUnit.OutboundQueue);
-	if (Index == 0xFFFFFFFFL)
+	if (unlikely(Index == 0xFFFFFFFFL))
 		Index = rx_readl(dev, MUnit.OutboundQueue);
-	if (Index != 0xFFFFFFFFL) {
+	if (likely(Index != 0xFFFFFFFFL)) {
 		do {
-			if (aac_intr_normal(dev, Index)) {
+			if (unlikely(aac_intr_normal(dev, Index))) {
 				rx_writel(dev, MUnit.OutboundQueue, Index);
 				rx_writel(dev, MUnit.ODR, DoorBellAdapterNormRespReady);
 			}
@@ -204,7 +204,7 @@
 		 */
 		msleep(1);
 	}
-	if (ok != 1) {
+	if (unlikely(ok != 1)) {
 		/*
 		 *	Restore interrupt mask even though we timed out
 		 */
@@ -214,15 +214,15 @@
 	/*
 	 *	Pull the synch status from Mailbox 0.
 	 */
-	if (status)
+	if (likely(status != NULL))
 		*status = readl(&dev->IndexRegs->Mailbox[0]);
-	if (r1)
+	if (likely(r1 != NULL))
 		*r1 = readl(&dev->IndexRegs->Mailbox[1]);
-	if (r2)
+	if (likely(r2 != NULL))
 		*r2 = readl(&dev->IndexRegs->Mailbox[2]);
-	if (r3)
+	if (likely(r3 != NULL))
 		*r3 = readl(&dev->IndexRegs->Mailbox[3]);
-	if (r4)
+	if (likely(r4 != NULL))
 		*r4 = readl(&dev->IndexRegs->Mailbox[4]);
 	/*
 	 *	Clear the synch command doorbell.
@@ -319,12 +319,12 @@
 	/*
 	 *	Check to see if the board failed any self tests.
 	 */
-	if (status & SELF_TEST_FAILED)
+	if (unlikely(status & SELF_TEST_FAILED))
 		return -1;
 	/*
 	 *	Check to see if the board panic'd.
 	 */
-	if (status & KERNEL_PANIC) {
+	if (unlikely(status & KERNEL_PANIC)) {
 		char * buffer;
 		struct POSTSTATUS {
 			__le32 Post_Command;
@@ -333,15 +333,15 @@
 		dma_addr_t paddr, baddr;
 		int ret;
 
-		if ((status & 0xFF000000L) == 0xBC000000L)
+		if (likely((status & 0xFF000000L) == 0xBC000000L))
 			return (status >> 16) & 0xFF;
 		buffer = pci_alloc_consistent(dev->pdev, 512, &baddr);
 		ret = -2;
-		if (buffer == NULL)
+		if (unlikely(buffer == NULL))
 			return ret;
 		post = pci_alloc_consistent(dev->pdev,
 		  sizeof(struct POSTSTATUS), &paddr);
-		if (post == NULL) {
+		if (unlikely(post == NULL)) {
 			pci_free_consistent(dev->pdev, 512, buffer, baddr);
 			return ret;
 		}
@@ -353,7 +353,7 @@
 		  NULL, NULL, NULL, NULL, NULL);
 		pci_free_consistent(dev->pdev, sizeof(struct POSTSTATUS),
 		  post, paddr);
-		if ((buffer[0] == '0') && ((buffer[1] == 'x') || (buffer[1] == 'X'))) {
+		if (likely((buffer[0] == '0') && ((buffer[1] == 'x') || (buffer[1] == 'X')))) {
 			ret = (buffer[2] <= '9') ? (buffer[2] - '0') : (buffer[2] - 'A' + 10);
 			ret <<= 4;
 			ret += (buffer[3] <= '9') ? (buffer[3] - '0') : (buffer[3] - 'A' + 10);
@@ -364,7 +364,7 @@
 	/*
 	 *	Wait for the adapter to be up and running.
 	 */
-	if (!(status & KERNEL_UP_AND_RUNNING))
+	if (unlikely(!(status & KERNEL_UP_AND_RUNNING)))
 		return -3;
 	/*
 	 *	Everything is OK
@@ -419,9 +419,9 @@
 	spin_unlock_irqrestore(q->lock, qflags);
 	for(;;) {
 		Index = rx_readl(dev, MUnit.InboundQueue);
-		if (Index == 0xFFFFFFFFL)
+		if (unlikely(Index == 0xFFFFFFFFL))
 			Index = rx_readl(dev, MUnit.InboundQueue);
-		if (Index != 0xFFFFFFFFL)
+		if (likely(Index != 0xFFFFFFFFL))
 			break;
 		if (--count == 0) {
 			spin_lock_irqsave(q->lock, qflags);
@@ -454,7 +454,7 @@
 		return 0;
 	}
 	dev->base = dev->regs.rx = ioremap(dev->scsi_host_ptr->base, size);
-	if (dev->base == NULL)
+	if (unlikely(dev->base == NULL))
 		return -1;
 	dev->IndexRegs = &dev->regs.rx->IndexRegs;
 	return 0;
@@ -526,13 +526,10 @@
 {
 	unsigned long start;
 	unsigned long status;
-	int instance;
-	const char * name;
+	int instance = dev->id;
+	const char * name = dev->name;
 
-	instance = dev->id;
-	name     = dev->name;
-
-	if (aac_adapter_ioremap(dev, dev->base_size)) {
+	if (unlikely(aac_adapter_ioremap(dev, dev->base_size))) {
 		printk(KERN_WARNING "%s: unable to map adapter.\n", name);
 		goto error_iounmap;
 	}
@@ -541,7 +538,7 @@
 	 *	Check to see if the board panic'd while booting.
 	 */
 	status = rx_readl(dev, MUnit.OMRx[0]);
-	if (status & KERNEL_PANIC) {
+	if (unlikely(status & KERNEL_PANIC)) {
 		if ((status = aac_rx_check_health(dev)) <= 0)
 			goto error_iounmap;
 		if (aac_rx_restart_adapter(dev, status))
@@ -551,14 +548,14 @@
 	 *	Check to see if the board failed any self tests.
 	 */
 	status = rx_readl(dev, MUnit.OMRx[0]);
-	if (status & SELF_TEST_FAILED) {
+	if (unlikely(status & SELF_TEST_FAILED)) {
 		printk(KERN_ERR "%s%d: adapter self-test failed.\n", dev->name, instance);
 		goto error_iounmap;
 	}
 	/*
 	 *	Check to see if the monitor panic'd while booting.
 	 */
-	if (status & MONITOR_PANIC) {
+	if (unlikely(status & MONITOR_PANIC)) {
 		printk(KERN_ERR "%s%d: adapter monitor panic.\n", dev->name, instance);
 		goto error_iounmap;
 	}
@@ -568,8 +565,7 @@
 	 */
 	while (!((status = rx_readl(dev, MUnit.OMRx[0])) & KERNEL_UP_AND_RUNNING))
 	{
-		if(time_after(jiffies, start+startup_timeout*HZ))
-		{
+		if(unlikely(time_after(jiffies, start+startup_timeout*HZ))) {
 			printk(KERN_ERR "%s%d: adapter kernel failed to start, init status = %lx.\n", 
 					dev->name, instance, status);
 			goto error_iounmap;
@@ -595,11 +591,11 @@
 	rx_writel(dev, MUnit.ODR, 0xffffffff);
 	aac_adapter_enable_int(dev);
 
-	if (aac_init_adapter(dev) == NULL)
+	if (unlikely(aac_init_adapter(dev) == NULL))
 		goto error_iounmap;
 	aac_adapter_comm(dev, dev->comm_interface);
-	if (request_irq(dev->scsi_host_ptr->irq, dev->a_ops.adapter_intr,
-			IRQF_SHARED|IRQF_DISABLED, "aacraid", dev) < 0) {
+	if (unlikely(request_irq(dev->scsi_host_ptr->irq, dev->a_ops.adapter_intr,
+			IRQF_SHARED|IRQF_DISABLED, "aacraid", dev) < 0)) {
 		printk(KERN_ERR "%s%d: Interrupt unavailable.\n",
 			name, instance);
 		goto error_iounmap;