| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 6 May 2016 18:08:02 +0900
From: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To: Ganesh Mahendran <opensource.ganesh@...il.com>
Cc: Minchan Kim <minchan@...nel.org>,
Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
Nitin Gupta <ngupta@...are.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Linux-MM <linux-mm@...ck.org>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm/zsmalloc: avoid unnecessary iteration in
get_pages_per_zspage()
On (05/06/16 12:25), Ganesh Mahendran wrote:
[..]
> > I agree with Sergey.
> > First of al, I appreciates your patch, Ganesh! But as Sergey pointed
> > out, I don't see why it improves current zsmalloc.
>
> This patch does not obviously improve zsmalloc.
> It just reduces unnecessary code path.
>
> From data provided by Sergey, 15 * (4 - 1) = 45 times loop will be avoided.
> So 45 times of below caculation will be reduced:
> ---
> zspage_size = i * PAGE_SIZE;
> waste = zspage_size % class_size;
> usedpc = (zspage_size - waste) * 100 / zspage_size;
>
> if (usedpc > max_usedpc) {
> ---
Hello,
I kinda believe we end up doing more work (instruction-count-wise),
actually. it adds 495 `cmp' for false case + 15 `cmp je' for true
case to eliminate 15 `mov cltd idiv mov sub imul cltd idiv cmp' *.
and it's not 45 iterations that we are getting rid of, but around 31:
not every class reaches it's ideal 100% ratio on the first iteration.
so, no, sorry, I don't think the patch really does what we want.
* by the way, we don't even need `cltd' in those calculations. the
reason why gcc puts cltd is because ZS_MAX_PAGES_PER_ZSPAGE has the
'wrong' data type. the patch to correct it is below (not a formal
patch).
** well, we force gcc to generate `worse' code in several more places.
for example, there is no need for `obj_idx' and `obj_offset' to be
`unsigned long', it can easly (and probably must) be `unsigned int',
or simply `int'. that can save some instructions in very-very hot paths:
add/remove: 0/0 grow/shrink: 1/6 up/down: 1/-27 (-26)
function old new delta
obj_free 234 235 +1
obj_to_location 45 44 -1
obj_malloc 234 233 -1
zs_malloc 817 815 -2
obj_idx_to_offset 32 28 -4
zs_unmap_object 556 551 -5
zs_compact 1611 1597 -14
I can cook a trivial patch later.
/*
* on x86_64, gcc 6.1. no idea what does the picture look like on ARM32.
* but smells like these two patches combined can make CPU a little less
* busy.
*/
=====================================================================
ZS_MAX_PAGES_PER_ZSPAGE defined as 'unsigned long' which forces
the compiler to generate unneeded signed extension instructions
`cltd' in several places. for instance:
711: 44 89 d0 mov %r10d,%eax
714: 99 cltd
715: 41 f7 fe idiv %r14d
718: 44 89 d0 mov %r10d,%eax
71b: 29 d0 sub %edx,%eax
71d: 6b c0 64 imul $0x64,%eax,%eax
720: 99 cltd
721: 41 f7 fa idiv %r10d
there is no reason to do this and ZS_MAX_PAGES_PER_ZSPAGE can
simply be 'int'.
the patch reduces the code size, a bit:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-25 (-25)
function old new delta
zs_malloc 842 817 -25
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
---
mm/zsmalloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index f9b58d1..1c28e0f6 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -78,7 +78,7 @@
* pages. ZS_MAX_ZSPAGE_ORDER defines upper limit on N.
*/
#define ZS_MAX_ZSPAGE_ORDER 2
-#define ZS_MAX_PAGES_PER_ZSPAGE (_AC(1, UL) << ZS_MAX_ZSPAGE_ORDER)
+#define ZS_MAX_PAGES_PER_ZSPAGE (1 << ZS_MAX_ZSPAGE_ORDER)
#define ZS_HANDLE_SIZE (sizeof(unsigned long))
--
2.8.2
Powered by blists - more mailing lists