Re: [PATCH] fix sparse warning from include/linux/mmzone.h

[Harvey Harrison <harvey.harrison@gmail.com>]

Maybe I'm beating a dead horse, but here's what I came up with:

include/linux/mmzone.h:640:22: warning: potentially expensive pointer subtraction

Using arch/x86/mm/highmem_32.o as an example, line 82, PageHighMem
expands to is_highmem.

	if (!PageHighMem(page))
		return page_address(page);

I've run through the current code, one of Linus' suggestions and Linus'
alternate version of my original patch without the stupid unsigned
long casts, using char *.  The best version appears to be the char *
version.  Code size will increase by one byte for each use of
is_highmem, but there is one fewer instruction (saves a sar over the
base code).  The reason for the code size increase is the two cmps
use 32-bit compares rather than 16 bit compares in the original due
to the sar beforehand. (-3 bytes for no sar +4 bytes for longer cmp)

It's a small thing, if you agree that the new code is better, I'll
send a patch.

Original:
static inline int is_highmem(struct zone *zone)
{
#ifdef CONFIG_HIGHMEM
	int zone_idx = zone - zone->zone_pgdat->node_zones;
	return zone_idx == ZONE_HIGHMEM ||
		(zone_idx == ZONE_MOVABLE && zone_movable_is_highmem());
#else
	return 0;
#endif
}

 207:	2b 80 8c 07 00 00    	sub    0x78c(%eax),%eax
 20d:	c1 f8 0b             	sar    $0xb,%eax
 210:	83 f8 02             	cmp    $0x2,%eax
 213:	74 16                	je     22b <kmap_atomic_prot+0x144>
 215:	83 f8 03             	cmp    $0x3,%eax
 218:	0f 85 8f 00 00 00    	jne    2ad <kmap_atomic_prot+0x1c6>
 21e:	83 3d 00 00 00 00 02 	cmpl   $0x2,0x0
 225:	0f 85 82 00 00 00    	jne    2ad <kmap_atomic_prot+0x1c6>
 22b:	64 a1 00 00 00 00    	mov    %fs:0x0,%eax

Linus' suggestion:
static inline int is_highmem(struct zone *zone)
{
#ifdef CONFIG_HIGHMEM
	struct zone *base = zone->zone_pgdat->node_zones;
	return zone == base + ZONE_HIGHMEM ||
		(zone == base + ZONE_MOVABLE && zone_movable_is_highmem());
#else
	return 0;
#endif
}

 202:	8d 90 00 00 00 00    	lea    0x0(%eax),%edx
 208:	8b 8a 8c 07 00 00    	mov    0x78c(%edx),%ecx
 20e:	8d 81 00 10 00 00    	lea    0x1000(%ecx),%eax
 214:	39 c2                	cmp    %eax,%edx
 216:	74 1b                	je     233 <kmap_atomic_prot+0x14c>
 218:	8d 81 00 18 00 00    	lea    0x1800(%ecx),%eax
 21e:	39 c2                	cmp    %eax,%edx
 220:	0f 85 8f 00 00 00    	jne    2b5 <kmap_atomic_prot+0x1ce>
 226:	83 3d 00 00 00 00 02 	cmpl   $0x2,0x0
 22d:	0f 85 82 00 00 00    	jne    2b5 <kmap_atomic_prot+0x1ce>
 233:	64 a1 00 00 00 00    	mov    %fs:0x0,%eax
 
An alternate suggestion (excuse the long line):

static inline int is_highmem(struct zone *zone)
{
#ifdef CONFIG_HIGHMEM
	int zone_off = (char *)zone - (char *)zone->zone_pgdat->node_zones;
	return zone_off == ZONE_HIGHMEM * sizeof(*zone) ||
		(zone_off == ZONE_MOVABLE * sizeof(*zone) && zone_movable_is_highmem());
#else
	return 0;
#endif
}

 207:	2b 80 8c 07 00 00    	sub    0x78c(%eax),%eax
 20d:	3d 00 10 00 00       	cmp    $0x1000,%eax
 212:	74 18                	je     22c <kmap_atomic_prot+0x145>
 214:	3d 00 18 00 00       	cmp    $0x1800,%eax
 219:	0f 85 8f 00 00 00    	jne    2ae <kmap_atomic_prot+0x1c7>
 21f:	83 3d 00 00 00 00 02 	cmpl   $0x2,0x0
 226:	0f 85 82 00 00 00    	jne    2ae <kmap_atomic_prot+0x1c7>
 22c:	64 a1 00 00 00 00    	mov    %fs:0x0,%eax

Cheers,

Harvey