* Re: [PATCH v3 3/4] dmaengine: Add Broadcom SBA RAID driver
From: Anup Patel @ 2017-02-13 9:13 UTC (permalink / raw)
To: Dan Williams
Cc: Vinod Koul, Rob Herring, Mark Rutland, Herbert Xu,
David S . Miller, Jassi Brar, Ray Jui, Scott Branden, Jon Mason,
Rob Rice, BCM Kernel Feedback, dmaengine@vger.kernel.org,
Device Tree, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-crypto, linux-raid
In-Reply-To: <CAPcyv4hE5gDiHhfaiHDHbhA2xKa45UdzKcSxnQXK-W92sr3Z1g@mail.gmail.com>
On Fri, Feb 10, 2017 at 11:20 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Fri, Feb 10, 2017 at 1:07 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>> The Broadcom stream buffer accelerator (SBA) provides offloading
>> capabilities for RAID operations. This SBA offload engine is
>> accessible via Broadcom SoC specific ring manager.
>>
>> This patch adds Broadcom SBA RAID driver which provides one
>> DMA device with RAID capabilities using one or more Broadcom
>> SoC specific ring manager channels. The SBA RAID driver in its
>> current shape implements memcpy, xor, and pq operations.
>>
>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>> drivers/dma/Kconfig | 13 +
>> drivers/dma/Makefile | 1 +
>> drivers/dma/bcm-sba-raid.c | 1711 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 1725 insertions(+)
>> create mode 100644 drivers/dma/bcm-sba-raid.c
>>
>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>> index 263495d..bf8fb84 100644
>> --- a/drivers/dma/Kconfig
>> +++ b/drivers/dma/Kconfig
>> @@ -99,6 +99,19 @@ config AXI_DMAC
>> controller is often used in Analog Device's reference designs for FPGA
>> platforms.
>>
>> +config BCM_SBA_RAID
>> + tristate "Broadcom SBA RAID engine support"
>> + depends on (ARM64 && MAILBOX && RAID6_PQ) || COMPILE_TEST
>> + select DMA_ENGINE
>> + select DMA_ENGINE_RAID
>> + select ASYNC_TX_ENABLE_CHANNEL_SWITCH
>
> ASYNC_TX_ENABLE_CHANNEL_SWITCH violates the DMA mapping API and
> Russell has warned it's especially problematic on ARM [1]. If you
> need channel switching for this offload engine to be useful then you
> need to move DMA mapping and channel switching responsibilities to MD
> itself.
>
> [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036753.html
Actually driver works fine with/without
ASYNC_TX_ENABLE_CHANNEL_SWITCH enabled
so I am fine with removing dependency on this config option.
>
>
> [..]
>> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> new file mode 100644
>> index 0000000..bab9918
>> --- /dev/null
>> +++ b/drivers/dma/bcm-sba-raid.c
>> @@ -0,0 +1,1711 @@
>> +/*
>> + * Copyright (C) 2017 Broadcom
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +/*
>> + * Broadcom SBA RAID Driver
>> + *
>> + * The Broadcom stream buffer accelerator (SBA) provides offloading
>> + * capabilities for RAID operations. The SBA offload engine is accessible
>> + * via Broadcom SoC specific ring manager. Two or more offload engines
>> + * can share same Broadcom SoC specific ring manager due to this Broadcom
>> + * SoC specific ring manager driver is implemented as a mailbox controller
>> + * driver and offload engine drivers are implemented as mallbox clients.
>> + *
>> + * Typically, Broadcom SoC specific ring manager will implement larger
>> + * number of hardware rings over one or more SBA hardware devices. By
>> + * design, the internal buffer size of SBA hardware device is limited
>> + * but all offload operations supported by SBA can be broken down into
>> + * multiple small size requests and executed parallely on multiple SBA
>> + * hardware devices for achieving high through-put.
>> + *
>> + * The Broadcom SBA RAID driver does not require any register programming
>> + * except submitting request to SBA hardware device via mailbox channels.
>> + * This driver implements a DMA device with one DMA channel using a set
>> + * of mailbox channels provided by Broadcom SoC specific ring manager
>> + * driver. To exploit parallelism (as described above), all DMA request
>> + * coming to SBA RAID DMA channel are broken down to smaller requests
>> + * and submitted to multiple mailbox channels in round-robin fashion.
>> + * For having more SBA DMA channels, we can create more SBA device nodes
>> + * in Broadcom SoC specific DTS based on number of hardware rings supported
>> + * by Broadcom SoC ring manager.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/list.h>
>> +#include <linux/mailbox_client.h>
>> +#include <linux/mailbox/brcm-message.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/raid/pq.h>
>> +
>> +#include "dmaengine.h"
>> +
>> +/* SBA command helper macros */
>> +#define SBA_DEC(_d, _s, _m) (((_d) >> (_s)) & (_m))
>> +#define SBA_ENC(_d, _v, _s, _m) \
>> + do { \
>> + (_d) &= ~((u64)(_m) << (_s)); \
>> + (_d) |= (((u64)(_v) & (_m)) << (_s)); \
>> + } while (0)
>
> Reusing a macro argument multiple times is problematic, consider
> SBA_ENC(..., arg++, ...), and hiding assignments in a macro make this
> hard to read. The compiler should inline it properly if you just make
> this a function that returns a value. You could also mark it __pure.
OK, I will make SBA_ENC as "static inline __pure" function.
>
> [..]
>> +
>> +static struct sba_request *sba_alloc_request(struct sba_device *sba)
>> +{
>> + unsigned long flags;
>> + struct sba_request *req = NULL;
>> +
>> + spin_lock_irqsave(&sba->reqs_lock, flags);
>> +
>> + if (!list_empty(&sba->reqs_free_list)) {
>> + req = list_first_entry(&sba->reqs_free_list,
>> + struct sba_request,
>> + node);
>
> You could use list_first_entry_or_null() here.
OK, will use this.
>
> [..]
>> +
>> +/* Note: Must be called with sba->reqs_lock held */
>> +static void _sba_pending_request(struct sba_device *sba,
>> + struct sba_request *req)
>> +{
>
> You can validate the locking assumptions here with
> lockdep_assert_head(sba->reqs_lock).
OK, will try this.
>
> [..]
>> +
>> +static void sba_cleanup_nonpending_requests(struct sba_device *sba)
>> +{
>> + unsigned long flags;
>> + struct sba_request *req, *req1;
>> +
>> + spin_lock_irqsave(&sba->reqs_lock, flags);
>> +
>> + /* Freeup all alloced request */
>> + list_for_each_entry_safe(req, req1, &sba->reqs_alloc_list, node) {
>> + _sba_free_request(sba, req);
>> + }
>> +
>> + /* Freeup all received request */
>> + list_for_each_entry_safe(req, req1, &sba->reqs_received_list, node) {
>> + _sba_free_request(sba, req);
>> + }
>> +
>> + /* Freeup all completed request */
>> + list_for_each_entry_safe(req, req1, &sba->reqs_completed_list, node) {
>> + _sba_free_request(sba, req);
>> + }
>> +
>> + /* Set all active requests as aborted */
>> + list_for_each_entry_safe(req, req1, &sba->reqs_active_list, node) {
>> + _sba_abort_request(sba, req);
>> + }
>
> In some parts of the driver you leave off unneeded braces like the for
> loop in sba_prep_dma_pq(), and in some case you include them. I'd say
> remove them if they're not necessary, but either way make it
> consistent across the driver.
I think I relied too much on checkpatch.pl to catch this
kind of coding-style issues.
I will fix this. Thanks for catching.
>
> [..]
>> +
>> +static struct dma_async_tx_descriptor *
>> +sba_prep_dma_pq(struct dma_chan *dchan, dma_addr_t *dst, dma_addr_t *src,
>> + u32 src_cnt, const u8 *scf, size_t len, unsigned long flags)
>> +{
>> + u32 i, dst_q_index;
>> + size_t req_len;
>> + bool slow = false;
>> + dma_addr_t off = 0;
>> + dma_addr_t *dst_p = NULL, *dst_q = NULL;
>> + struct sba_device *sba = to_sba_device(dchan);
>> + struct sba_request *first = NULL, *req;
>> +
>> + /* Sanity checks */
>> + if (unlikely(src_cnt > sba->max_pq_srcs))
>> + return NULL;
>> + for (i = 0; i < src_cnt; i++)
>> + if (sba->max_pq_coefs <= raid6_gflog[scf[i]])
>> + slow = true;
>
> Thanks, yes, I do think this is cleaner here than in async_tx itself.
>
> [..]
>> +static void sba_receive_message(struct mbox_client *cl, void *msg)
>> +{
>> + unsigned long flags;
>> + struct brcm_message *m = msg;
>> + struct sba_request *req = m->ctx, *req1;
>> + struct sba_device *sba = req->sba;
>> +
>> + /* Error count if message has error */
>> + if (m->error < 0) {
>> + dev_err(sba->dev, "%s got message with error %d",
>> + dma_chan_name(&sba->dma_chan), m->error);
>> + }
>> +
>> + /* Mark request as received */
>> + sba_received_request(req);
>> +
>> + /* Wait for all chained requests to be completed */
>> + if (atomic_dec_return(&req->first->next_pending_count))
>> + goto done;
>> +
>> + /* Point to first request */
>> + req = req->first;
>> +
>> + /* Update request */
>> + if (req->state == SBA_REQUEST_STATE_RECEIVED)
>> + sba_dma_tx_actions(req);
>> + else
>> + sba_free_chained_requests(req);
>> +
>> + spin_lock_irqsave(&sba->reqs_lock, flags);
>> +
>> + /* Re-check all completed request waiting for 'ack' */
>> + list_for_each_entry_safe(req, req1, &sba->reqs_completed_list, node) {
>> + spin_unlock_irqrestore(&sba->reqs_lock, flags);
>> + sba_dma_tx_actions(req);
>
> You've now required all callback paths to be hardirq safe whereas
> previously the callbacks only assumed softirq exclusion. Have you run
> this with CONFIG_PROVE_LOCKING enabled?
We have run stress tests on driver with multiple threads
trying to submit txn.
I will certainly try CONFIG_PROVE_LOCKING to be
double sure.
Thanks,
Anup
^ permalink raw reply
* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: Minchan Kim @ 2017-02-13 0:03 UTC (permalink / raw)
To: Sven Schmidt
Cc: ebiggers3, akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky,
gregkh, linux-kernel, herbert, davem, linux-crypto, anton, ccross,
keescook, tony.luck
In-Reply-To: <1486898178-17125-1-git-send-email-4sschmid@informatik.uni-hamburg.de>
Hi Sven,
On Sun, Feb 12, 2017 at 12:16:17PM +0100, Sven Schmidt wrote:
>
>
>
> On 02/10/2017 01:13 AM, Minchan Kim wrote:
> > Hello Sven,
> >
> > On Thu, Feb 09, 2017 at 11:56:17AM +0100, Sven Schmidt wrote:
> >> Hey Minchan,
> >>
> >> On Thu, Feb 09, 2017 at 08:31:21AM +0900, Minchan Kim wrote:
> >>> Hello Sven,
> >>>
> >>> On Sun, Feb 05, 2017 at 08:09:03PM +0100, Sven Schmidt wrote:
> >>>>
> >>>> This patchset is for updating the LZ4 compression module to a version based
> >>>> on LZ4 v1.7.3 allowing to use the fast compression algorithm aka LZ4 fast
> >>>> which provides an "acceleration" parameter as a tradeoff between
> >>>> high compression ratio and high compression speed.
> >>>>
> >>>> We want to use LZ4 fast in order to support compression in lustre
> >>>> and (mostly, based on that) investigate data reduction techniques in behalf of
> >>>> storage systems.
> >>>>
> >>>> Also, it will be useful for other users of LZ4 compression, as with LZ4 fast
> >>>> it is possible to enable applications to use fast and/or high compression
> >>>> depending on the usecase.
> >>>> For instance, ZRAM is offering a LZ4 backend and could benefit from an updated
> >>>> LZ4 in the kernel.
> >>>>
> >>>> LZ4 homepage: http://www.lz4.org/
> >>>> LZ4 source repository: https://github.com/lz4/lz4
> >>>> Source version: 1.7.3
> >>>>
> >>>> Benchmark (taken from [1], Core i5-4300U @1.9GHz):
> >>>> ----------------|--------------|----------------|----------
> >>>> Compressor | Compression | Decompression | Ratio
> >>>> ----------------|--------------|----------------|----------
> >>>> memcpy | 4200 MB/s | 4200 MB/s | 1.000
> >>>> LZ4 fast 50 | 1080 MB/s | 2650 MB/s | 1.375
> >>>> LZ4 fast 17 | 680 MB/s | 2220 MB/s | 1.607
> >>>> LZ4 fast 5 | 475 MB/s | 1920 MB/s | 1.886
> >>>> LZ4 default | 385 MB/s | 1850 MB/s | 2.101
> >>>>
> >>>> [1] http://fastcompression.blogspot.de/2015/04/sampling-or-faster-lz4.html
> >>>>
> >>>> [PATCH 1/5] lib: Update LZ4 compressor module
> >>>> [PATCH 2/5] lib/decompress_unlz4: Change module to work with new LZ4 module version
> >>>> [PATCH 3/5] crypto: Change LZ4 modules to work with new LZ4 module version
> >>>> [PATCH 4/5] fs/pstore: fs/squashfs: Change usage of LZ4 to work with new LZ4 version
> >>>> [PATCH 5/5] lib/lz4: Remove back-compat wrappers
> >>>
> >>> Today, I did zram-lz4 performance test with fio in current mmotm and
> >>> found it makes regression about 20%.
> >>>
> >>> "lz4-update" means current mmots(git://git.cmpxchg.org/linux-mmots.git) so
> >>> applied your 5 patches. (But now sure current mmots has recent uptodate
> >>> patches)
> >>> "revert" means I reverted your 5 patches in current mmots.
> >>>
> >>> revert lz4-update
> >>>
> >>> seq-write 1547 1339 86.55%
> >>> rand-write 22775 19381 85.10%
> >>> seq-read 7035 5589 79.45%
> >>> rand-read 78556 68479 87.17%
> >>> mixed-seq(R) 1305 1066 81.69%
> >>> mixed-seq(W) 1205 984 81.66%
> >>> mixed-rand(R) 17421 14993 86.06%
> >>> mixed-rand(W) 17391 14968 86.07%
> >>
> >> which parts of the output (as well as units) are these values exactly?
> >> I did not work with fio until now, so I think I might ask before misinterpreting my results.
> >
> > It is IOPS.
> >
> >>
> >>> My fio description file
> >>>
> >>> [global]
> >>> bs=4k
> >>> ioengine=sync
> >>> size=100m
> >>> numjobs=1
> >>> group_reporting
> >>> buffer_compress_percentage=30
> >>> scramble_buffers=0
> >>> filename=/dev/zram0
> >>> loops=10
> >>> fsync_on_close=1
> >>>
> >>> [seq-write]
> >>> bs=64k
> >>> rw=write
> >>> stonewall
> >>>
> >>> [rand-write]
> >>> rw=randwrite
> >>> stonewall
> >>>
> >>> [seq-read]
> >>> bs=64k
> >>> rw=read
> >>> stonewall
> >>>
> >>> [rand-read]
> >>> rw=randread
> >>> stonewall
> >>>
> >>> [mixed-seq]
> >>> bs=64k
> >>> rw=rw
> >>> stonewall
> >>>
> >>> [mixed-rand]
> >>> rw=randrw
> >>> stonewall
> >>>
> >>
> >> Great, this makes it easy for me to reproduce your test.
> >
> > If you have trouble to reproduce, feel free to ask me. I'm happy to test it. :)
> >
> > Thanks!
> >
>
> Hi Minchan,
>
> I will send an updated patch as a reply to this E-Mail. Would be really grateful If you'd test it and provide feedback!
> The patch should be applied to the current mmots tree.
>
> In fact, the updated LZ4 _is_ slower than the current one in kernel. But I was not able to reproduce such large regressions
> as you did. I now tried to define FORCE_INLINE as Eric suggested. I also inlined some functions which weren't in upstream LZ4,
> but are defined as macros in the current kernel LZ4. The approach to replace LZ4_ARCH64 with the function call _seemed_ to behave
> worse than the macro, so I withdrew the change.
>
> The main difference is, that I replaced the read32/read16/write... etc. functions using memcpy with the other ones defined
> in upstream LZ4 (which can be switched using a macro).
> The comment of the author stated, that they're as fast as the memcpy variants (or faster), but not as portable
> (which does not matter since we're not dependent for multiple compilers).
>
> In my tests, this version is mostly as fast as the current kernel LZ4.
With a patch you sent, I cannot see enhancement so I wanted to dig in and
found how I was really careless.
I have tested both test with CONFIG_KASAN. OMG. With disabling it, I don't
see any regression any more. So, I'm really really *sorry* about noise and
wasting your time. However, I am curious why KASAN makes such difference.
The reason I tested new updated lz4 is description says lz4 fast and
want to use it in zram. How can I do that? and How faster it is compared
to old?
Thanks for you work!
^ permalink raw reply
* Re: linux-next: build warnings after merge of the crypto tree
From: Stephen Rothwell @ 2017-02-12 23:42 UTC (permalink / raw)
To: Herbert Xu
Cc: linux-next, linux-kernel, Cyrille Pitchen,
Linux Crypto Mailing List
In-Reply-To: <20170211105621.GJ855@gondor.apana.org.au>
Hi Herbert,
On Sat, 11 Feb 2017 18:56:21 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Fri, Feb 10, 2017 at 02:12:51PM +1100, Stephen Rothwell wrote:
> >
> > I am still getting these warnings ... I have seen no updates to the
> > crypot tree since Feb 2.
>
> Sorry Stephen. I have now applied Arnd's fixes for this problem
> and it should be pushed out.
Thanks, its much cleaner now. :-)
--
Cheers,
Stephen Rothwell
^ permalink raw reply
* Re: [PATCH] lz4: fix performance regressions
From: Eric Biggers @ 2017-02-12 23:38 UTC (permalink / raw)
To: Sven Schmidt
Cc: minchan, akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky,
gregkh, linux-kernel, herbert, davem, linux-crypto, anton, ccross,
keescook, tony.luck
In-Reply-To: <1486898178-17125-2-git-send-email-4sschmid@informatik.uni-hamburg.de>
Hi Sven,
On Sun, Feb 12, 2017 at 12:16:18PM +0100, Sven Schmidt wrote:
> /*-************************************
> * Reading and writing into memory
> **************************************/
> +typedef union {
> + U16 u16;
> + U32 u32;
> + size_t uArch;
> +} __packed unalign;
>
> -static inline U16 LZ4_read16(const void *memPtr)
> +static FORCE_INLINE __maybe_unused U16 LZ4_read16(const void *ptr)
> {
> - U16 val;
> -
> - memcpy(&val, memPtr, sizeof(val));
> -
> - return val;
> + return ((const unalign *)ptr)->u16;
> }
>
> -static inline U32 LZ4_read32(const void *memPtr)
> +static FORCE_INLINE __maybe_unused U32 LZ4_read32(const void *ptr)
> {
> - U32 val;
> -
> - memcpy(&val, memPtr, sizeof(val));
> -
> - return val;
> + return ((const unalign *)ptr)->u32;
> }
>
> -static inline size_t LZ4_read_ARCH(const void *memPtr)
> +static FORCE_INLINE __maybe_unused size_t LZ4_read_ARCH(const void *ptr)
> {
> - size_t val;
> -
> - memcpy(&val, memPtr, sizeof(val));
> -
> - return val;
> + return ((const unalign *)ptr)->uArch;
> }
>
> -static inline void LZ4_write16(void *memPtr, U16 value)
> +static FORCE_INLINE __maybe_unused void LZ4_write16(void *memPtr, U16 value)
> {
> - memcpy(memPtr, &value, sizeof(value));
> + ((unalign *)memPtr)->u16 = value;
> }
>
> -static inline void LZ4_write32(void *memPtr, U32 value)
> -{
> - memcpy(memPtr, &value, sizeof(value));
> +static FORCE_INLINE __maybe_unused void LZ4_write32(void *memPtr, U32 value) {
> + ((unalign *)memPtr)->u32 = value;
> }
>
> -static inline U16 LZ4_readLE16(const void *memPtr)
> +static FORCE_INLINE __maybe_unused U16 LZ4_readLE16(const void *memPtr)
> {
> -#ifdef __LITTLE_ENDIAN__
> +#if LZ4_LITTLE_ENDIAN
> return LZ4_read16(memPtr);
> #else
> const BYTE *p = (const BYTE *)memPtr;
> @@ -137,19 +143,19 @@ static inline U16 LZ4_readLE16(const void *memPtr)
> #endif
> }
Since upstream LZ4 is intended to be compiled at -O3, this may allow it to get
away with using memcpy() for unaligned memory accesses. The reason it uses
memcpy() is that, other than a byte-by-byte copy, it is the only portable way to
express unaligned memory accesses. But the Linux kernel is sometimes compiled
optimized for size (-Os), and I wouldn't be *too* surprised if some of the
memcpy()'s don't always get inlined then, which could be causing the performance
regression being observed. (Of course, this could be verified by checking
whether CONFIG_CC_OPTIMIZE_FOR_SIZE=y is set, then reading the assembly.)
But I don't think accessing a __packed structure directly is the right
alternative. Instead, Linux already includes macros for unaligned memory
accesses which have been optimized for every supported architecture. Those
should just be used instead, e.g. like this:
static FORCE_INLINE U16 LZ4_read16(const void *ptr)
{
return get_unaligned((const u16 *)ptr);
}
static FORCE_INLINE U32 LZ4_read32(const void *ptr)
{
return get_unaligned((const u32 *)ptr);
}
static FORCE_INLINE size_t LZ4_read_ARCH(const void *ptr)
{
return get_unaligned((const size_t *)ptr);
}
static FORCE_INLINE void LZ4_write16(void *memPtr, U16 value)
{
put_unaligned(value, (u16 *)memPtr);
}
static FORCE_INLINE void LZ4_write32(void *memPtr, U32 value)
{
put_unaligned(value, (u32 *)memPtr);
}
static FORCE_INLINE U16 LZ4_readLE16(const void *memPtr)
{
return get_unaligned_le16(memPtr);
}
static FORCE_INLINE void LZ4_writeLE16(void *memPtr, U16 value)
{
return put_unaligned_le16(value, memPtr);
}
static FORCE_INLINE void LZ4_copy8(void *dst, const void *src)
{
if (LZ4_64bits()) {
u64 a = get_unaligned((const u64 *)src);
put_unaligned(a, (u64 *)dst);
} else {
u32 a = get_unaligned((const u32 *)src);
u32 b = get_unaligned((const u32 *)src + 1);
put_unaligned(a, (u32 *)dst);
put_unaligned(b, (u32 *)dst + 1);
}
}
Note that I dropped __maybe_unused as it's not needed on inline functions.
That should be done everywhere else the patch proposes to add it too.
> -#if LZ4_ARCH64
> -#ifdef __BIG_ENDIAN__
> -#define LZ4_NBCOMMONBYTES(val) (__builtin_clzll(val) >> 3)
> +static FORCE_INLINE unsigned int LZ4_NbCommonBytes(register size_t val)
> +{
> +#if LZ4_LITTLE_ENDIAN
> +#if LZ4_ARCH64 /* 64 Bits Little Endian */
> +#if defined(LZ4_FORCE_SW_BITCOUNT)
> + static const int DeBruijnBytePos[64] = {
> + 0, 0, 0, 0, 0, 1, 1, 2, 0, 3, 1, 3, 1, 4, 2, 7,
> + 0, 2, 3, 6, 1, 5, 3, 5, 1, 3, 4, 4, 2, 5, 6, 7,
> + 7, 0, 1, 2, 3, 3, 4, 6, 2, 6, 5, 5, 3, 4, 5, 6,
> + 7, 1, 2, 4, 6, 4, 4, 5, 7, 2, 6, 5, 7, 6, 7, 7
> + };
> +
> + return DeBruijnBytePos[((U64)((val & -(long long)val)
> + * 0x0218A392CDABBD3FULL)) >> 58];
> #else
> -#define LZ4_NBCOMMONBYTES(val) (__builtin_ctzll(val) >> 3)
> -#endif
> + return (__builtin_ctzll((U64)val) >> 3);
> +#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
> +#else /* 32 Bits Little Endian */
> +#if defined(LZ4_FORCE_SW_BITCOUNT)
> + static const int DeBruijnBytePos[32] = {
> + 0, 0, 3, 0, 3, 1, 3, 0, 3, 2, 2, 1, 3, 2, 0, 1,
> + 3, 3, 1, 2, 2, 2, 2, 0, 3, 1, 2, 0, 1, 0, 1, 1
> + };
> +
> + return DeBruijnBytePos[((U32)((val & -(S32)val)
> + * 0x077CB531U)) >> 27];
> #else
> -#ifdef __BIG_ENDIAN__
> -#define LZ4_NBCOMMONBYTES(val) (__builtin_clz(val) >> 3)
> + return (__builtin_ctz((U32)val) >> 3);
> +#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
> +#endif /* LZ4_ARCH64 */
> +#else /* Big Endian */
> +#if LZ4_ARCH64 /* 64 Bits Big Endian */
> +#if defined(LZ4_FORCE_SW_BITCOUNT)
> + unsigned int r;
> +
> + if (!(val >> 32)) {
> + r = 4;
> + } else {
> + r = 0;
> + val >>= 32;
> + }
> +
> + if (!(val >> 16)) {
> + r += 2;
> + val >>= 8;
> + } else {
> + val >>= 24;
> + }
> +
> + r += (!val);
> +
> + return r;
> #else
> -#define LZ4_NBCOMMONBYTES(val) (__builtin_ctz(val) >> 3)
> -#endif
> -#endif
> + return (__builtin_clzll((U64)val) >> 3);
> +#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
> +#else /* 32 Bits Big Endian */
> +#if defined(LZ4_FORCE_SW_BITCOUNT)
> + unsigned int r;
> +
> + if (!(val >> 16)) {
> + r = 2;
> + val >>= 8;
> + } else {
> + r = 0;
> + val >>= 24;
> + }
> +
> + r += (!val);
> +
> + return r;
> +#else
> + return (__builtin_clz((U32)val) >> 3);
> +#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
> +#endif /* LZ4_ARCH64 */
> +#endif /* LZ4_LITTLE_ENDIAN */
> +}
The reason LZ4_NbCommonBytes() in upstream LZ4 is so complicated is that it
needs to provide portable fallbacks that work on *any* platform and compiler.
This isn't needed in the Linux kernel, and it should just call the functions
already defined that do the right thing:
static FORCE_INLINE unsigned int LZ4_NbCommonBytes(register size_t val)
{
if (LZ4_isLittleEndian())
return __ffs(val) >> 3;
else
return (BITS_PER_LONG - 1 - __fls(val)) >> 3;
}
To be clear, when I said that upstream LZ4 shouldn't generally be changed, I'm
primarily talking about the core code, not the platform-specific parts. What we
need to do is define platform-specific stuff, like LZ4_read*(), LZ4_write*(),
LZ4_NbCommonBytes(), LZ4_64bits(), and FORCE_INLINE, in a way that makes sense
for the Linux kernel and the environment it's compiled in. Also I think it's
fine, and maybe even necessary for performance, to *add* inline or FORCE_INLINE
in some places too, given that LZ4 in Linux may get compiled with a lower
optimization level than that intended to be used for upstream LZ4 --- though it
may be worth considering updating the Makefile to just always compile the LZ4
files with -O3 instead. What should be avoided is making unnecessary changes to
the *users* of the platform-specific code or to the core (de)compression
parameters or templates.
Eric
^ permalink raw reply
* Re: [PATCH] lz4: fix performance regressions
From: Willy Tarreau @ 2017-02-12 21:41 UTC (permalink / raw)
To: Sven Schmidt
Cc: minchan, ebiggers3, akpm, bongkyu.kim, rsalvaterra,
sergey.senozhatsky, gregkh, linux-kernel, herbert, davem,
linux-crypto, anton, ccross, keescook, tony.luck
In-Reply-To: <20170212152000.GA24704@bierbaron.springfield.local>
On Sun, Feb 12, 2017 at 04:20:00PM +0100, Sven Schmidt wrote:
> On Sun, Feb 12, 2017 at 02:05:08PM +0100, Willy Tarreau wrote:
> > Hi Sven,
> >
> > On Sun, Feb 12, 2017 at 12:16:18PM +0100, Sven Schmidt wrote:
> > > Fix performance regressions compared to current kernel LZ4
> >
> > Your patch contains mostly style cleanups which certainly are welcome
> > but make the whole patch hard to review. These cleanups would have been
> > better into a separate, preliminary patch IMHO.
> >
> > Regards,
> > Willy
>
> Hi Willy,
>
> the problem was, I wanted to compare my version to the upstream LZ4 to find bugs (as with my last patch version: wrong indentation in LZ4HC
> in two for loops). But since the LZ4 code is a pain to read, I made additional style cleanups "on the way".
Oh I can easily understand!
> Hope you can manage to review the patch though, because it is difficult to separate the cleanups now.
When I need to split a patch into pieces, usually what I do is that I
revert it, re-apply it without committing, then "git add -p", validate
all the hunks to be taken as the first patch (ie here the cleanups),
commit, then commit the rest as a separate one. It seems to me that the
fix is in the last few hunks though I'm not sure yet.
Thanks,
Willy
^ permalink raw reply
* Re: [PATCH] lz4: fix performance regressions
From: Sven Schmidt @ 2017-02-12 15:20 UTC (permalink / raw)
To: Willy Tarreau
Cc: minchan, ebiggers3, akpm, bongkyu.kim, rsalvaterra,
sergey.senozhatsky, gregkh, linux-kernel, herbert, davem,
linux-crypto, anton, ccross, keescook, tony.luck
In-Reply-To: <20170212130508.GA27710@1wt.eu>
On Sun, Feb 12, 2017 at 02:05:08PM +0100, Willy Tarreau wrote:
> Hi Sven,
>
> On Sun, Feb 12, 2017 at 12:16:18PM +0100, Sven Schmidt wrote:
> > Fix performance regressions compared to current kernel LZ4
>
> Your patch contains mostly style cleanups which certainly are welcome
> but make the whole patch hard to review. These cleanups would have been
> better into a separate, preliminary patch IMHO.
>
> Regards,
> Willy
Hi Willy,
the problem was, I wanted to compare my version to the upstream LZ4 to find bugs (as with my last patch version: wrong indentation in LZ4HC
in two for loops). But since the LZ4 code is a pain to read, I made additional style cleanups "on the way".
Hope you can manage to review the patch though, because it is difficult to separate the cleanups now.
Please feel free to ask if you stumble upon something.
Greetings,
Sven
^ permalink raw reply
* [PATCH RFC] crypto: testmgr drop wrong init_completion
From: Nicholas Mc Guire @ 2017-02-12 15:05 UTC (permalink / raw)
To: Herbert Xu
Cc: David S. Miller, Tadeusz Struk, linux-crypto, linux-kernel,
Nicholas Mc Guire
init_completion() is called here to reinitialize a completion object
that was already re-initialized in wait_async_op() by
reinit_completion() if complete (via tcrypt_complete()) had been called
and wait_for_completion() returned, so no need to reinit it here.
Fixes: commit 946cc46372dc ("crypto: testmgr - add tests vectors for RSA")
Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
Found by experimental coccinelle script
./crypto/testmgr.c:2174:1-16: WARNING: possible duplicate init_completion
Only based on code review and no testing. In case I am overlooking something
and the re-initialization of the completion object is actually needed it
should be using reinit_completion() and not init_completion() anyway.
But as wait_async_op() will leave with the completion object re-initialized
it really should not be needed here (found no path in between that could
have called completion()).
Patch was only compile tested with: x86_64_defconfig (implies cryptomgr-y)
Patch is against linux-4.10-rc6 (localversion-next is next-20170210)
crypto/testmgr.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 98eb097..15fb453 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -2171,7 +2171,6 @@ static int test_akcipher_one(struct crypto_akcipher *tfm,
sg_init_one(&src, xbuf[0], vecs->c_size);
sg_init_one(&dst, outbuf_dec, out_len_max);
- init_completion(&result.completion);
akcipher_request_set_crypt(req, &src, &dst, vecs->c_size, out_len_max);
/* Run RSA decrypt - m = c^d mod n;*/
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] lz4: fix performance regressions
From: Willy Tarreau @ 2017-02-12 13:05 UTC (permalink / raw)
To: Sven Schmidt
Cc: minchan, ebiggers3, akpm, bongkyu.kim, rsalvaterra,
sergey.senozhatsky, gregkh, linux-kernel, herbert, davem,
linux-crypto, anton, ccross, keescook, tony.luck
In-Reply-To: <1486898178-17125-2-git-send-email-4sschmid@informatik.uni-hamburg.de>
Hi Sven,
On Sun, Feb 12, 2017 at 12:16:18PM +0100, Sven Schmidt wrote:
> Fix performance regressions compared to current kernel LZ4
Your patch contains mostly style cleanups which certainly are welcome
but make the whole patch hard to review. These cleanups would have been
better into a separate, preliminary patch IMHO.
Regards,
Willy
^ permalink raw reply
* [PATCH] lz4: fix performance regressions
From: Sven Schmidt @ 2017-02-12 11:16 UTC (permalink / raw)
To: minchan
Cc: ebiggers3, akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky,
gregkh, linux-kernel, herbert, davem, linux-crypto, anton, ccross,
keescook, tony.luck, Sven Schmidt
In-Reply-To: <1486898178-17125-1-git-send-email-4sschmid@informatik.uni-hamburg.de>
Fix performance regressions compared to current kernel LZ4
Signed-off-by: Sven Schmidt <4sschmid@informatik.uni-hamburg.de>
---
include/linux/lz4.h | 2 +-
lib/lz4/lz4_compress.c | 157 +++++++++++++++++++++++-------------
lib/lz4/lz4_decompress.c | 50 ++++++++----
lib/lz4/lz4defs.h | 203 ++++++++++++++++++++++++++++++++---------------
lib/lz4/lz4hc_compress.c | 8 +-
5 files changed, 281 insertions(+), 139 deletions(-)
diff --git a/include/linux/lz4.h b/include/linux/lz4.h
index a3912d7..394e3d9 100644
--- a/include/linux/lz4.h
+++ b/include/linux/lz4.h
@@ -82,7 +82,7 @@
/*-************************************************************************
* STREAMING CONSTANTS AND STRUCTURES
**************************************************************************/
-#define LZ4_STREAMSIZE_U64 ((1 << (LZ4_MEMORY_USAGE-3)) + 4)
+#define LZ4_STREAMSIZE_U64 ((1 << (LZ4_MEMORY_USAGE - 3)) + 4)
#define LZ4_STREAMSIZE (LZ4_STREAMSIZE_U64 * sizeof(unsigned long long))
#define LZ4_STREAMHCSIZE 262192
diff --git a/lib/lz4/lz4_compress.c b/lib/lz4/lz4_compress.c
index 697dbda..2cbbf99 100644
--- a/lib/lz4/lz4_compress.c
+++ b/lib/lz4/lz4_compress.c
@@ -39,27 +39,33 @@
#include <linux/kernel.h>
#include <asm/unaligned.h>
+static const int LZ4_minLength = (MFLIMIT + 1);
+static const int LZ4_64Klimit = ((64 * KB) + (MFLIMIT - 1));
+
/*-******************************
* Compression functions
********************************/
-static U32 LZ4_hash4(U32 sequence, tableType_t const tableType)
+static FORCE_INLINE U32 LZ4_hash4(
+ U32 sequence,
+ tableType_t const tableType)
{
if (tableType == byU16)
return ((sequence * 2654435761U)
- >> ((MINMATCH*8) - (LZ4_HASHLOG + 1)));
+ >> ((MINMATCH * 8) - (LZ4_HASHLOG + 1)));
else
return ((sequence * 2654435761U)
- >> ((MINMATCH*8) - LZ4_HASHLOG));
+ >> ((MINMATCH * 8) - LZ4_HASHLOG));
}
-#if LZ4_ARCH64
-static U32 LZ4_hash5(U64 sequence, tableType_t const tableType)
+static FORCE_INLINE __maybe_unused U32 LZ4_hash5(
+ U64 sequence,
+ tableType_t const tableType)
{
const U32 hashLog = (tableType == byU16)
? LZ4_HASHLOG + 1
: LZ4_HASHLOG;
-#ifdef __LITTLE_ENDIAN__
+#if LZ4_LITTLE_ENDIAN
static const U64 prime5bytes = 889523592379ULL;
return (U32)(((sequence << 24) * prime5bytes) >> (64 - hashLog));
@@ -69,9 +75,10 @@ static U32 LZ4_hash5(U64 sequence, tableType_t const tableType)
return (U32)(((sequence >> 24) * prime8bytes) >> (64 - hashLog));
#endif
}
-#endif
-static U32 LZ4_hashPosition(const void *p, tableType_t tableType)
+static FORCE_INLINE U32 LZ4_hashPosition(
+ const void *p,
+ tableType_t const tableType)
{
#if LZ4_ARCH64
if (tableType == byU32)
@@ -81,8 +88,12 @@ static U32 LZ4_hashPosition(const void *p, tableType_t tableType)
return LZ4_hash4(LZ4_read32(p), tableType);
}
-static void LZ4_putPositionOnHash(const BYTE *p, U32 h, void *tableBase,
- tableType_t const tableType, const BYTE *srcBase)
+static void LZ4_putPositionOnHash(
+ const BYTE *p,
+ U32 h,
+ void *tableBase,
+ tableType_t const tableType,
+ const BYTE *srcBase)
{
switch (tableType) {
case byPtr:
@@ -109,16 +120,22 @@ static void LZ4_putPositionOnHash(const BYTE *p, U32 h, void *tableBase,
}
}
-static inline void LZ4_putPosition(const BYTE *p, void *tableBase,
- tableType_t tableType, const BYTE *srcBase)
+static FORCE_INLINE void LZ4_putPosition(
+ const BYTE *p,
+ void *tableBase,
+ tableType_t tableType,
+ const BYTE *srcBase)
{
U32 const h = LZ4_hashPosition(p, tableType);
LZ4_putPositionOnHash(p, h, tableBase, tableType, srcBase);
}
-static const BYTE *LZ4_getPositionOnHash(U32 h, void *tableBase,
- tableType_t tableType, const BYTE *srcBase)
+static const BYTE *LZ4_getPositionOnHash(
+ U32 h,
+ void *tableBase,
+ tableType_t tableType,
+ const BYTE *srcBase)
{
if (tableType == byPtr) {
const BYTE **hashTable = (const BYTE **) tableBase;
@@ -135,12 +152,16 @@ static const BYTE *LZ4_getPositionOnHash(U32 h, void *tableBase,
{
/* default, to ensure a return */
const U16 * const hashTable = (U16 *) tableBase;
+
return hashTable[h] + srcBase;
}
}
-static inline const BYTE *LZ4_getPosition(const BYTE *p, void *tableBase,
- tableType_t tableType, const BYTE *srcBase)
+static FORCE_INLINE const BYTE *LZ4_getPosition(
+ const BYTE *p,
+ void *tableBase,
+ tableType_t tableType,
+ const BYTE *srcBase)
{
U32 const h = LZ4_hashPosition(p, tableType);
@@ -152,7 +173,7 @@ static inline const BYTE *LZ4_getPosition(const BYTE *p, void *tableBase,
* LZ4_compress_generic() :
* inlined, to ensure branches are decided at compilation time
*/
-static inline int LZ4_compress_generic(
+static FORCE_INLINE int LZ4_compress_generic(
LZ4_stream_t_internal * const dictPtr,
const char * const source,
char * const dest,
@@ -187,6 +208,7 @@ static inline int LZ4_compress_generic(
/* Unsupported inputSize, too large (or negative) */
return 0;
}
+
switch (dict) {
case noDict:
default:
@@ -216,7 +238,8 @@ static inline int LZ4_compress_generic(
/* First Byte */
LZ4_putPosition(ip, dictPtr->hashTable, tableType, base);
- ip++; forwardH = LZ4_hashPosition(ip, tableType);
+ ip++;
+ forwardH = LZ4_hashPosition(ip, tableType);
/* Main Loop */
for ( ; ; ) {
@@ -227,15 +250,14 @@ static inline int LZ4_compress_generic(
{
const BYTE *forwardIp = ip;
unsigned int step = 1;
- unsigned int searchMatchNb = acceleration
- << LZ4_skipTrigger;
+ unsigned int searchMatchNb = acceleration << LZ4_SKIPTRIGGER;
do {
U32 const h = forwardH;
ip = forwardIp;
forwardIp += step;
- step = (searchMatchNb++ >> LZ4_skipTrigger);
+ step = (searchMatchNb++ >> LZ4_SKIPTRIGGER);
if (unlikely(forwardIp > mflimit))
goto _last_literals;
@@ -243,6 +265,7 @@ static inline int LZ4_compress_generic(
match = LZ4_getPositionOnHash(h,
dictPtr->hashTable,
tableType, base);
+
if (dict == usingExtDict) {
if (match < (const BYTE *)source) {
refDelta = dictDelta;
@@ -251,11 +274,12 @@ static inline int LZ4_compress_generic(
refDelta = 0;
lowLimit = (const BYTE *)source;
} }
+
forwardH = LZ4_hashPosition(forwardIp,
tableType);
+
LZ4_putPositionOnHash(ip, h, dictPtr->hashTable,
tableType, base);
-
} while (((dictIssue == dictSmall)
? (match < lowRefLimit)
: 0)
@@ -268,31 +292,34 @@ static inline int LZ4_compress_generic(
/* Catch up */
while (((ip > anchor) & (match + refDelta > lowLimit))
- && (unlikely(ip[-1] == match[refDelta - 1]))) {
+ && (unlikely(ip[-1] == match[refDelta - 1]))) {
ip--;
match--;
- }
+ }
/* Encode Literals */
{
unsigned const int litLength = (unsigned int)(ip - anchor);
token = op++;
+
if ((outputLimited) &&
/* Check output buffer overflow */
(unlikely(op + litLength +
(2 + 1 + LASTLITERALS) +
- (litLength/255) > olimit)))
+ (litLength / 255) > olimit)))
return 0;
+
if (litLength >= RUN_MASK) {
int len = (int)litLength - RUN_MASK;
- *token = (RUN_MASK<<ML_BITS);
- for (; len >= 255 ; len -= 255)
+ *token = (RUN_MASK << ML_BITS);
+
+ for (; len >= 255; len -= 255)
*op++ = 255;
*op++ = (BYTE)len;
} else
- *token = (BYTE)(litLength<<ML_BITS);
+ *token = (BYTE)(litLength << ML_BITS);
/* Copy Literals */
LZ4_wildCopy(op, anchor, op + litLength);
@@ -301,7 +328,8 @@ static inline int LZ4_compress_generic(
_next_match:
/* Encode Offset */
- LZ4_writeLE16(op, (U16)(ip - match)); op += 2;
+ LZ4_writeLE16(op, (U16)(ip - match));
+ op += 2;
/* Encode MatchLength */
{
@@ -313,11 +341,15 @@ static inline int LZ4_compress_generic(
match += refDelta;
limit = ip + (dictEnd - match);
+
if (limit > matchlimit)
limit = matchlimit;
+
matchCode = LZ4_count(ip + MINMATCH,
match + MINMATCH, limit);
+
ip += MINMATCH + matchCode;
+
if (ip == limit) {
unsigned const int more = LZ4_count(ip,
(const BYTE *)source,
@@ -336,17 +368,20 @@ static inline int LZ4_compress_generic(
/* Check output buffer overflow */
(unlikely(op +
(1 + LASTLITERALS) +
- (matchCode>>8) > olimit)))
+ (matchCode >> 8) > olimit)))
return 0;
+
if (matchCode >= ML_MASK) {
*token += ML_MASK;
matchCode -= ML_MASK;
LZ4_write32(op, 0xFFFFFFFF);
- while (matchCode >= 4*255) {
+
+ while (matchCode >= 4 * 255) {
op += 4;
LZ4_write32(op, 0xFFFFFFFF);
- matchCode -= 4*255;
+ matchCode -= 4 * 255;
}
+
op += matchCode / 255;
*op++ = (BYTE)(matchCode % 255);
} else
@@ -365,6 +400,7 @@ static inline int LZ4_compress_generic(
/* Test next position */
match = LZ4_getPosition(ip, dictPtr->hashTable,
tableType, base);
+
if (dict == usingExtDict) {
if (match < (const BYTE *)source) {
refDelta = dictDelta;
@@ -374,7 +410,9 @@ static inline int LZ4_compress_generic(
lowLimit = (const BYTE *)source;
}
}
+
LZ4_putPosition(ip, dictPtr->hashTable, tableType, base);
+
if (((dictIssue == dictSmall) ? (match >= lowRefLimit) : 1)
&& (match + MAX_DISTANCE >= ip)
&& (LZ4_read32(match + refDelta) == LZ4_read32(ip))) {
@@ -395,18 +433,21 @@ static inline int LZ4_compress_generic(
if ((outputLimited) &&
/* Check output buffer overflow */
((op - (BYTE *)dest) + lastRun + 1 +
- ((lastRun + 255 - RUN_MASK)/255) > (U32)maxOutputSize))
+ ((lastRun + 255 - RUN_MASK) / 255) > (U32)maxOutputSize))
return 0;
+
if (lastRun >= RUN_MASK) {
size_t accumulator = lastRun - RUN_MASK;
*op++ = RUN_MASK << ML_BITS;
- for (; accumulator >= 255 ; accumulator -= 255)
+ for (; accumulator >= 255; accumulator -= 255)
*op++ = 255;
*op++ = (BYTE) accumulator;
} else {
- *op++ = (BYTE)(lastRun<<ML_BITS);
+ *op++ = (BYTE)(lastRun << ML_BITS);
}
+
memcpy(op, anchor, lastRun);
+
op += lastRun;
}
@@ -414,23 +455,27 @@ static inline int LZ4_compress_generic(
return (int) (((char *)op) - dest);
}
-static int LZ4_compress_fast_extState(void *state, const char *source, char *dest,
- int inputSize, int maxOutputSize, int acceleration)
+static int LZ4_compress_fast_extState(
+ void *state,
+ const char *source,
+ char *dest,
+ int inputSize,
+ int maxOutputSize,
+ int acceleration)
{
- #if LZ4_ARCH64
- tableType_t tableType = byU32;
- #else
- tableType_t tableType = byPtr;
- #endif
-
LZ4_stream_t_internal *ctx = &((LZ4_stream_t *)state)->internal_donotuse;
+#if LZ4_ARCH64
+ const tableType_t tableType = byU32;
+#else
+ const tableType_t tableType = byPtr;
+#endif
LZ4_resetStream((LZ4_stream_t *)state);
if (acceleration < 1)
acceleration = LZ4_ACCELERATION_DEFAULT;
- if (maxOutputSize >= LZ4_compressBound(inputSize)) {
+ if (maxOutputSize >= LZ4_COMPRESSBOUND(inputSize)) {
if (inputSize < LZ4_64Klimit)
return LZ4_compress_generic(ctx, source,
dest, inputSize, 0,
@@ -474,7 +519,6 @@ EXPORT_SYMBOL(LZ4_compress_default);
/*-******************************
* *_destSize() variant
********************************/
-
static int LZ4_compress_destSize_generic(
LZ4_stream_t_internal * const ctx,
const char * const src,
@@ -529,14 +573,14 @@ static int LZ4_compress_destSize_generic(
{
const BYTE *forwardIp = ip;
unsigned int step = 1;
- unsigned int searchMatchNb = 1 << LZ4_skipTrigger;
+ unsigned int searchMatchNb = 1 << LZ4_SKIPTRIGGER;
do {
U32 h = forwardH;
ip = forwardIp;
forwardIp += step;
- step = (searchMatchNb++ >> LZ4_skipTrigger);
+ step = (searchMatchNb++ >> LZ4_SKIPTRIGGER);
if (unlikely(forwardIp > mflimit))
goto _last_literals;
@@ -559,8 +603,9 @@ static int LZ4_compress_destSize_generic(
while ((ip > anchor)
&& (match > lowLimit)
&& (unlikely(ip[-1] == match[-1]))) {
- ip--; match--;
- }
+ ip--;
+ match--;
+ }
/* Encode Literal length */
{
@@ -644,11 +689,11 @@ static int LZ4_compress_destSize_generic(
size_t lastRunSize = (size_t)(iend - anchor);
if (op + 1 /* token */
- + ((lastRunSize + 240)/255) /* litLength */
+ + ((lastRunSize + 240) / 255) /* litLength */
+ lastRunSize /* literals */ > oend) {
/* adapt lastRunSize to fill 'dst' */
lastRunSize = (oend - op) - 1;
- lastRunSize -= (lastRunSize + 240)/255;
+ lastRunSize -= (lastRunSize + 240) / 255;
}
ip = anchor + lastRunSize;
@@ -656,7 +701,7 @@ static int LZ4_compress_destSize_generic(
size_t accumulator = lastRunSize - RUN_MASK;
*op++ = RUN_MASK << ML_BITS;
- for (; accumulator >= 255 ; accumulator -= 255)
+ for (; accumulator >= 255; accumulator -= 255)
*op++ = 255;
*op++ = (BYTE) accumulator;
} else {
@@ -675,14 +720,14 @@ static int LZ4_compress_destSize_extState(LZ4_stream_t *state, const char *src,
char *dst, int *srcSizePtr, int targetDstSize)
{
#if LZ4_ARCH64
- tableType_t tableType = byU32;
+ const tableType_t tableType = byU32;
#else
- tableType_t tableType = byPtr;
+ const tableType_t tableType = byPtr;
#endif
LZ4_resetStream(state);
- if (targetDstSize >= LZ4_compressBound(*srcSizePtr)) {
+ if (targetDstSize >= LZ4_COMPRESSBOUND(*srcSizePtr)) {
/* compression success is guaranteed */
return LZ4_compress_fast_extState(
state, src, dst, *srcSizePtr,
@@ -847,7 +892,7 @@ int LZ4_compress_fast_continue(LZ4_stream_t *LZ4_stream, const char *source,
result = LZ4_compress_generic(
streamPtr, source, dest, inputSize,
maxOutputSize, limitedOutput, byU32,
- withPrefix64k, dictSmall, acceleration);
+ withPrefix64k, dictSmall, acceleration);
} else {
result = LZ4_compress_generic(
streamPtr, source, dest, inputSize,
diff --git a/lib/lz4/lz4_decompress.c b/lib/lz4/lz4_decompress.c
index a7731ba..3bfc2f6 100644
--- a/lib/lz4/lz4_decompress.c
+++ b/lib/lz4/lz4_decompress.c
@@ -49,8 +49,8 @@
* Note that it is important this generic function is really inlined,
* in order to remove useless branches during compilation optimization.
*/
-static inline int LZ4_decompress_generic(
- const char *const source,
+static FORCE_INLINE int LZ4_decompress_generic(
+ const char * const source,
char * const dest,
int inputSize,
/*
@@ -180,22 +180,28 @@ static inline int LZ4_decompress_generic(
goto _output_error;
}
}
+
memcpy(op, ip, length);
ip += length;
op += length;
/* Necessarily EOF, due to parsing restrictions */
break;
}
+
LZ4_wildCopy(op, ip, cpy);
- ip += length; op = cpy;
+ ip += length;
+ op = cpy;
/* get offset */
- offset = LZ4_readLE16(ip); ip += 2;
+ offset = LZ4_readLE16(ip);
+ ip += 2;
match = op - offset;
+
if ((checkOffset) && (unlikely(match < lowLimit))) {
/* Error : offset outside buffers */
goto _output_error;
}
+
/* costs ~1%; silence an msan warning when offset == 0 */
LZ4_write32(op, (U32)offset);
@@ -205,11 +211,14 @@ static inline int LZ4_decompress_generic(
unsigned int s;
do {
- s = *ip++;
- if ((endOnInput) && (ip > iend - LASTLITERALS))
- goto _output_error;
- length += s;
+ s = *ip++;
+
+ if ((endOnInput) && (ip > iend - LASTLITERALS))
+ goto _output_error;
+
+ length += s;
} while (s == 255);
+
if ((safeDecode)
&& unlikely(
(size_t)(op + length) < (size_t)op)) {
@@ -217,6 +226,7 @@ static inline int LZ4_decompress_generic(
goto _output_error;
}
}
+
length += MINMATCH;
/* check external dictionary */
@@ -227,12 +237,13 @@ static inline int LZ4_decompress_generic(
}
if (length <= (size_t)(lowPrefix - match)) {
- /*
- * match can be copied as a single segment
- * from external dictionary
- */
- memmove(op, dictEnd - (lowPrefix - match), length);
- op += length;
+ /*
+ * match can be copied as a single segment
+ * from external dictionary
+ */
+ memmove(op, dictEnd - (lowPrefix - match),
+ length);
+ op += length;
} else {
/*
* match encompass external
@@ -256,11 +267,13 @@ static inline int LZ4_decompress_generic(
op += restSize;
}
}
+
continue;
}
/* copy match within block */
cpy = op + length;
+
if (unlikely(offset < 8)) {
const int dec64 = dec64table[offset];
@@ -272,7 +285,8 @@ static inline int LZ4_decompress_generic(
memcpy(op + 4, match, 4);
match -= dec64;
} else {
- LZ4_copy8(op, match); match += 8;
+ LZ4_copy8(op, match);
+ match += 8;
}
op += 8;
@@ -287,18 +301,22 @@ static inline int LZ4_decompress_generic(
*/
goto _output_error;
}
+
if (op < oCopyLimit) {
LZ4_wildCopy(op, match, oCopyLimit);
match += oCopyLimit - op;
op = oCopyLimit;
}
+
while (op < cpy)
*op++ = *match++;
} else {
LZ4_copy8(op, match);
+
if (length > 16)
LZ4_wildCopy(op + 8, match + 8, cpy);
}
+
op = cpy; /* correction */
}
@@ -438,7 +456,7 @@ int LZ4_decompress_fast_continue(LZ4_streamDecode_t *LZ4_streamDecode,
* These decoding functions work the same as "_continue" ones,
* the dictionary must be explicitly provided within parameters
*/
-static inline int LZ4_decompress_usingDict_generic(const char *source,
+static FORCE_INLINE int LZ4_decompress_usingDict_generic(const char *source,
char *dest, int compressedSize, int maxOutputSize, int safe,
const char *dictStart, int dictSize)
{
diff --git a/lib/lz4/lz4defs.h b/lib/lz4/lz4defs.h
index 23e1a1b..47ef42b 100644
--- a/lib/lz4/lz4defs.h
+++ b/lib/lz4/lz4defs.h
@@ -38,14 +38,7 @@
#include <asm/unaligned.h>
#include <linux/string.h> /* memset, memcpy */
-/*
- * Detects 64 bits mode
-*/
-#if defined(CONFIG_64BIT)
-#define LZ4_ARCH64 1
-#else
-#define LZ4_ARCH64 0
-#endif
+#define FORCE_INLINE __always_inline
/*-************************************
* Basic Types
@@ -60,14 +53,38 @@ typedef uint64_t U64;
typedef uintptr_t uptrval;
/*-************************************
+ * Architecture specifics
+ **************************************/
+#if defined(CONFIG_64BIT)
+#define LZ4_ARCH64 1
+#else
+#define LZ4_ARCH64 0
+#endif
+
+#if defined(__LITTLE_ENDIAN)
+#define LZ4_LITTLE_ENDIAN 1
+#else
+#define LZ4_LITTLE_ENDIAN 0
+#endif
+
+/*
+ * LZ4_FORCE_SW_BITCOUNT
+ * Define this parameter if your target system
+ * does not support hardware bit count
+ */
+/* #define LZ4_FORCE_SW_BITCOUNT */
+
+/*-************************************
* Constants
**************************************/
#define MINMATCH 4
#define WILDCOPYLENGTH 8
#define LASTLITERALS 5
-#define MFLIMIT (WILDCOPYLENGTH+MINMATCH)
-static const int LZ4_minLength = (MFLIMIT+1);
+#define MFLIMIT (WILDCOPYLENGTH + MINMATCH)
+
+/* Increase this value ==> compression run slower on incompressible data */
+#define LZ4_SKIPTRIGGER 6
#define KB (1<<10)
#define MB (1<<20)
@@ -82,53 +99,42 @@ static const int LZ4_minLength = (MFLIMIT+1);
#define RUN_BITS (8-ML_BITS)
#define RUN_MASK ((1U<<RUN_BITS)-1)
-static const int LZ4_64Klimit = ((64 * KB) + (MFLIMIT-1));
-static const U32 LZ4_skipTrigger = 6;
-
/*-************************************
* Reading and writing into memory
**************************************/
+typedef union {
+ U16 u16;
+ U32 u32;
+ size_t uArch;
+} __packed unalign;
-static inline U16 LZ4_read16(const void *memPtr)
+static FORCE_INLINE __maybe_unused U16 LZ4_read16(const void *ptr)
{
- U16 val;
-
- memcpy(&val, memPtr, sizeof(val));
-
- return val;
+ return ((const unalign *)ptr)->u16;
}
-static inline U32 LZ4_read32(const void *memPtr)
+static FORCE_INLINE __maybe_unused U32 LZ4_read32(const void *ptr)
{
- U32 val;
-
- memcpy(&val, memPtr, sizeof(val));
-
- return val;
+ return ((const unalign *)ptr)->u32;
}
-static inline size_t LZ4_read_ARCH(const void *memPtr)
+static FORCE_INLINE __maybe_unused size_t LZ4_read_ARCH(const void *ptr)
{
- size_t val;
-
- memcpy(&val, memPtr, sizeof(val));
-
- return val;
+ return ((const unalign *)ptr)->uArch;
}
-static inline void LZ4_write16(void *memPtr, U16 value)
+static FORCE_INLINE __maybe_unused void LZ4_write16(void *memPtr, U16 value)
{
- memcpy(memPtr, &value, sizeof(value));
+ ((unalign *)memPtr)->u16 = value;
}
-static inline void LZ4_write32(void *memPtr, U32 value)
-{
- memcpy(memPtr, &value, sizeof(value));
+static FORCE_INLINE __maybe_unused void LZ4_write32(void *memPtr, U32 value) {
+ ((unalign *)memPtr)->u32 = value;
}
-static inline U16 LZ4_readLE16(const void *memPtr)
+static FORCE_INLINE __maybe_unused U16 LZ4_readLE16(const void *memPtr)
{
-#ifdef __LITTLE_ENDIAN__
+#if LZ4_LITTLE_ENDIAN
return LZ4_read16(memPtr);
#else
const BYTE *p = (const BYTE *)memPtr;
@@ -137,19 +143,19 @@ static inline U16 LZ4_readLE16(const void *memPtr)
#endif
}
-static inline void LZ4_writeLE16(void *memPtr, U16 value)
+static FORCE_INLINE __maybe_unused void LZ4_writeLE16(void *memPtr, U16 value)
{
-#ifdef __LITTLE_ENDIAN__
+#if LZ4_LITTLE_ENDIAN
LZ4_write16(memPtr, value);
#else
BYTE *p = (BYTE *)memPtr;
p[0] = (BYTE) value;
- p[1] = (BYTE)(value>>8);
+ p[1] = (BYTE)(value >> 8);
#endif
}
-static inline void LZ4_copy8(void *dst, const void *src)
+static FORCE_INLINE void LZ4_copy8(void *dst, const void *src)
{
memcpy(dst, src, 8);
}
@@ -158,7 +164,8 @@ static inline void LZ4_copy8(void *dst, const void *src)
* customized variant of memcpy,
* which can overwrite up to 7 bytes beyond dstEnd
*/
-static inline void LZ4_wildCopy(void *dstPtr, const void *srcPtr, void *dstEnd)
+static FORCE_INLINE void LZ4_wildCopy(void *dstPtr,
+ const void *srcPtr, void *dstEnd)
{
BYTE *d = (BYTE *)dstPtr;
const BYTE *s = (const BYTE *)srcPtr;
@@ -171,49 +178,121 @@ static inline void LZ4_wildCopy(void *dstPtr, const void *srcPtr, void *dstEnd)
} while (d < e);
}
-#if LZ4_ARCH64
-#ifdef __BIG_ENDIAN__
-#define LZ4_NBCOMMONBYTES(val) (__builtin_clzll(val) >> 3)
+static FORCE_INLINE unsigned int LZ4_NbCommonBytes(register size_t val)
+{
+#if LZ4_LITTLE_ENDIAN
+#if LZ4_ARCH64 /* 64 Bits Little Endian */
+#if defined(LZ4_FORCE_SW_BITCOUNT)
+ static const int DeBruijnBytePos[64] = {
+ 0, 0, 0, 0, 0, 1, 1, 2, 0, 3, 1, 3, 1, 4, 2, 7,
+ 0, 2, 3, 6, 1, 5, 3, 5, 1, 3, 4, 4, 2, 5, 6, 7,
+ 7, 0, 1, 2, 3, 3, 4, 6, 2, 6, 5, 5, 3, 4, 5, 6,
+ 7, 1, 2, 4, 6, 4, 4, 5, 7, 2, 6, 5, 7, 6, 7, 7
+ };
+
+ return DeBruijnBytePos[((U64)((val & -(long long)val)
+ * 0x0218A392CDABBD3FULL)) >> 58];
#else
-#define LZ4_NBCOMMONBYTES(val) (__builtin_ctzll(val) >> 3)
-#endif
+ return (__builtin_ctzll((U64)val) >> 3);
+#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
+#else /* 32 Bits Little Endian */
+#if defined(LZ4_FORCE_SW_BITCOUNT)
+ static const int DeBruijnBytePos[32] = {
+ 0, 0, 3, 0, 3, 1, 3, 0, 3, 2, 2, 1, 3, 2, 0, 1,
+ 3, 3, 1, 2, 2, 2, 2, 0, 3, 1, 2, 0, 1, 0, 1, 1
+ };
+
+ return DeBruijnBytePos[((U32)((val & -(S32)val)
+ * 0x077CB531U)) >> 27];
#else
-#ifdef __BIG_ENDIAN__
-#define LZ4_NBCOMMONBYTES(val) (__builtin_clz(val) >> 3)
+ return (__builtin_ctz((U32)val) >> 3);
+#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
+#endif /* LZ4_ARCH64 */
+#else /* Big Endian */
+#if LZ4_ARCH64 /* 64 Bits Big Endian */
+#if defined(LZ4_FORCE_SW_BITCOUNT)
+ unsigned int r;
+
+ if (!(val >> 32)) {
+ r = 4;
+ } else {
+ r = 0;
+ val >>= 32;
+ }
+
+ if (!(val >> 16)) {
+ r += 2;
+ val >>= 8;
+ } else {
+ val >>= 24;
+ }
+
+ r += (!val);
+
+ return r;
#else
-#define LZ4_NBCOMMONBYTES(val) (__builtin_ctz(val) >> 3)
-#endif
-#endif
+ return (__builtin_clzll((U64)val) >> 3);
+#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
+#else /* 32 Bits Big Endian */
+#if defined(LZ4_FORCE_SW_BITCOUNT)
+ unsigned int r;
+
+ if (!(val >> 16)) {
+ r = 2;
+ val >>= 8;
+ } else {
+ r = 0;
+ val >>= 24;
+ }
+
+ r += (!val);
+
+ return r;
+#else
+ return (__builtin_clz((U32)val) >> 3);
+#endif /* defined(LZ4_FORCE_SW_BITCOUNT) */
+#endif /* LZ4_ARCH64 */
+#endif /* LZ4_LITTLE_ENDIAN */
+}
-static inline unsigned int LZ4_count(const BYTE *pIn, const BYTE *pMatch,
+static FORCE_INLINE __maybe_unused unsigned int LZ4_count(
+ const BYTE *pIn,
+ const BYTE *pMatch,
const BYTE *pInLimit)
{
const BYTE *const pStart = pIn;
- while (likely(pIn < pInLimit-(STEPSIZE-1))) {
- size_t diff = LZ4_read_ARCH(pMatch) ^ LZ4_read_ARCH(pIn);
+ while (likely(pIn < pInLimit - (STEPSIZE - 1))) {
+ size_t const diff = LZ4_read_ARCH(pMatch) ^ LZ4_read_ARCH(pIn);
if (!diff) {
pIn += STEPSIZE;
pMatch += STEPSIZE;
continue;
}
- pIn += LZ4_NBCOMMONBYTES(diff);
+
+ pIn += LZ4_NbCommonBytes(diff);
+
return (unsigned int)(pIn - pStart);
}
-#ifdef LZ4_ARCH64
- if ((pIn < (pInLimit-3))
+#if LZ4_ARCH64
+ if ((pIn < (pInLimit - 3))
&& (LZ4_read32(pMatch) == LZ4_read32(pIn))) {
- pIn += 4; pMatch += 4;
+ pIn += 4;
+ pMatch += 4;
}
#endif
- if ((pIn < (pInLimit-1))
+
+ if ((pIn < (pInLimit - 1))
&& (LZ4_read16(pMatch) == LZ4_read16(pIn))) {
- pIn += 2; pMatch += 2;
+ pIn += 2;
+ pMatch += 2;
}
+
if ((pIn < pInLimit) && (*pMatch == *pIn))
pIn++;
+
return (unsigned int)(pIn - pStart);
}
diff --git a/lib/lz4/lz4hc_compress.c b/lib/lz4/lz4hc_compress.c
index 8363292..c7271a1 100644
--- a/lib/lz4/lz4hc_compress.c
+++ b/lib/lz4/lz4hc_compress.c
@@ -71,7 +71,7 @@ static void LZ4HC_init(LZ4HC_CCtx_internal *hc4, const BYTE *start)
}
/* Update chains up to ip (excluded) */
-static inline void LZ4HC_Insert(LZ4HC_CCtx_internal *hc4,
+static FORCE_INLINE void LZ4HC_Insert(LZ4HC_CCtx_internal *hc4,
const BYTE *ip)
{
U16 * const chainTable = hc4->chainTable;
@@ -96,7 +96,7 @@ static inline void LZ4HC_Insert(LZ4HC_CCtx_internal *hc4,
hc4->nextToUpdate = target;
}
-static inline int LZ4HC_InsertAndFindBestMatch(
+static FORCE_INLINE int LZ4HC_InsertAndFindBestMatch(
LZ4HC_CCtx_internal *hc4, /* Index table will be updated */
const BYTE *ip,
const BYTE * const iLimit,
@@ -165,7 +165,7 @@ static inline int LZ4HC_InsertAndFindBestMatch(
return (int)ml;
}
-static inline int LZ4HC_InsertAndGetWiderMatch(
+static FORCE_INLINE int LZ4HC_InsertAndGetWiderMatch(
LZ4HC_CCtx_internal *hc4,
const BYTE * const ip,
const BYTE * const iLowLimit,
@@ -259,7 +259,7 @@ static inline int LZ4HC_InsertAndGetWiderMatch(
return longest;
}
-static inline int LZ4HC_encodeSequence(
+static FORCE_INLINE int LZ4HC_encodeSequence(
const BYTE **ip,
BYTE **op,
const BYTE **anchor,
^ permalink raw reply related
* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: Sven Schmidt @ 2017-02-12 11:16 UTC (permalink / raw)
To: minchan
Cc: ebiggers3, akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky,
gregkh, linux-kernel, herbert, davem, linux-crypto, anton, ccross,
keescook, tony.luck
In-Reply-To: <20170210001311.GA25078@bbox>
On 02/10/2017 01:13 AM, Minchan Kim wrote:
> Hello Sven,
>
> On Thu, Feb 09, 2017 at 11:56:17AM +0100, Sven Schmidt wrote:
>> Hey Minchan,
>>
>> On Thu, Feb 09, 2017 at 08:31:21AM +0900, Minchan Kim wrote:
>>> Hello Sven,
>>>
>>> On Sun, Feb 05, 2017 at 08:09:03PM +0100, Sven Schmidt wrote:
>>>>
>>>> This patchset is for updating the LZ4 compression module to a version based
>>>> on LZ4 v1.7.3 allowing to use the fast compression algorithm aka LZ4 fast
>>>> which provides an "acceleration" parameter as a tradeoff between
>>>> high compression ratio and high compression speed.
>>>>
>>>> We want to use LZ4 fast in order to support compression in lustre
>>>> and (mostly, based on that) investigate data reduction techniques in behalf of
>>>> storage systems.
>>>>
>>>> Also, it will be useful for other users of LZ4 compression, as with LZ4 fast
>>>> it is possible to enable applications to use fast and/or high compression
>>>> depending on the usecase.
>>>> For instance, ZRAM is offering a LZ4 backend and could benefit from an updated
>>>> LZ4 in the kernel.
>>>>
>>>> LZ4 homepage: http://www.lz4.org/
>>>> LZ4 source repository: https://github.com/lz4/lz4
>>>> Source version: 1.7.3
>>>>
>>>> Benchmark (taken from [1], Core i5-4300U @1.9GHz):
>>>> ----------------|--------------|----------------|----------
>>>> Compressor | Compression | Decompression | Ratio
>>>> ----------------|--------------|----------------|----------
>>>> memcpy | 4200 MB/s | 4200 MB/s | 1.000
>>>> LZ4 fast 50 | 1080 MB/s | 2650 MB/s | 1.375
>>>> LZ4 fast 17 | 680 MB/s | 2220 MB/s | 1.607
>>>> LZ4 fast 5 | 475 MB/s | 1920 MB/s | 1.886
>>>> LZ4 default | 385 MB/s | 1850 MB/s | 2.101
>>>>
>>>> [1] http://fastcompression.blogspot.de/2015/04/sampling-or-faster-lz4.html
>>>>
>>>> [PATCH 1/5] lib: Update LZ4 compressor module
>>>> [PATCH 2/5] lib/decompress_unlz4: Change module to work with new LZ4 module version
>>>> [PATCH 3/5] crypto: Change LZ4 modules to work with new LZ4 module version
>>>> [PATCH 4/5] fs/pstore: fs/squashfs: Change usage of LZ4 to work with new LZ4 version
>>>> [PATCH 5/5] lib/lz4: Remove back-compat wrappers
>>>
>>> Today, I did zram-lz4 performance test with fio in current mmotm and
>>> found it makes regression about 20%.
>>>
>>> "lz4-update" means current mmots(git://git.cmpxchg.org/linux-mmots.git) so
>>> applied your 5 patches. (But now sure current mmots has recent uptodate
>>> patches)
>>> "revert" means I reverted your 5 patches in current mmots.
>>>
>>> revert lz4-update
>>>
>>> seq-write 1547 1339 86.55%
>>> rand-write 22775 19381 85.10%
>>> seq-read 7035 5589 79.45%
>>> rand-read 78556 68479 87.17%
>>> mixed-seq(R) 1305 1066 81.69%
>>> mixed-seq(W) 1205 984 81.66%
>>> mixed-rand(R) 17421 14993 86.06%
>>> mixed-rand(W) 17391 14968 86.07%
>>
>> which parts of the output (as well as units) are these values exactly?
>> I did not work with fio until now, so I think I might ask before misinterpreting my results.
>
> It is IOPS.
>
>>
>>> My fio description file
>>>
>>> [global]
>>> bs=4k
>>> ioengine=sync
>>> size=100m
>>> numjobs=1
>>> group_reporting
>>> buffer_compress_percentage=30
>>> scramble_buffers=0
>>> filename=/dev/zram0
>>> loops=10
>>> fsync_on_close=1
>>>
>>> [seq-write]
>>> bs=64k
>>> rw=write
>>> stonewall
>>>
>>> [rand-write]
>>> rw=randwrite
>>> stonewall
>>>
>>> [seq-read]
>>> bs=64k
>>> rw=read
>>> stonewall
>>>
>>> [rand-read]
>>> rw=randread
>>> stonewall
>>>
>>> [mixed-seq]
>>> bs=64k
>>> rw=rw
>>> stonewall
>>>
>>> [mixed-rand]
>>> rw=randrw
>>> stonewall
>>>
>>
>> Great, this makes it easy for me to reproduce your test.
>
> If you have trouble to reproduce, feel free to ask me. I'm happy to test it. :)
>
> Thanks!
>
Hi Minchan,
I will send an updated patch as a reply to this E-Mail. Would be really grateful If you'd test it and provide feedback!
The patch should be applied to the current mmots tree.
In fact, the updated LZ4 _is_ slower than the current one in kernel. But I was not able to reproduce such large regressions
as you did. I now tried to define FORCE_INLINE as Eric suggested. I also inlined some functions which weren't in upstream LZ4,
but are defined as macros in the current kernel LZ4. The approach to replace LZ4_ARCH64 with the function call _seemed_ to behave
worse than the macro, so I withdrew the change.
The main difference is, that I replaced the read32/read16/write... etc. functions using memcpy with the other ones defined
in upstream LZ4 (which can be switched using a macro).
The comment of the author stated, that they're as fast as the memcpy variants (or faster), but not as portable
(which does not matter since we're not dependent for multiple compilers).
In my tests, this version is mostly as fast as the current kernel LZ4.
Thank you!
Sven
^ permalink raw reply
* Клиентские базы +79139230330 Skype: prodawez390 Email: prodawez393@gmail.com Узнайте подробнее!
From: linux-crypto @ 2017-02-12 4:44 UTC (permalink / raw)
To: linux-crypto
Клиентские базы +79139230330 Skype: prodawez390 Email: prodawez393@gmail.com Узнайте подробнее!
^ permalink raw reply
* [PATCH 2/2] crypto: ccm - drop unnecessary minimum 32-bit alignment
From: Ard Biesheuvel @ 2017-02-11 19:25 UTC (permalink / raw)
To: linux-crypto, herbert; +Cc: Ard Biesheuvel
In-Reply-To: <1486841122-1686-1-git-send-email-ard.biesheuvel@linaro.org>
The CCM driver forces 32-bit alignment even if the underlying ciphers
don't care about alignment. This is because crypto_xor() used to require
this, but since this is no longer the case, drop the hardcoded minimum
of 32 bits.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
crypto/ccm.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/crypto/ccm.c b/crypto/ccm.c
index 24c26ab052ca..442848807a52 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -525,8 +525,7 @@ static int crypto_ccm_create_common(struct crypto_template *tmpl,
ctr->base.cra_priority) / 2;
inst->alg.base.cra_blocksize = 1;
inst->alg.base.cra_alignmask = mac->base.cra_alignmask |
- ctr->base.cra_alignmask |
- (__alignof__(u32) - 1);
+ ctr->base.cra_alignmask;
inst->alg.ivsize = 16;
inst->alg.chunksize = crypto_skcipher_alg_chunksize(ctr);
inst->alg.maxauthsize = 16;
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] crypto: ccm - honour alignmask of subordinate MAC cipher
From: Ard Biesheuvel @ 2017-02-11 19:25 UTC (permalink / raw)
To: linux-crypto, herbert; +Cc: Ard Biesheuvel
The CCM driver was recently updated to defer the MAC part of the algorithm
to a dedicated crypto transform, and a template for instantiating such
transforms was added at the same time.
However, this new cbcmac template fails to take the alignmask of the
encapsulated cipher into account, which may result in buffer addresses
being passed down that are not sufficiently aligned.
So update the code to ensure that the digest buffer in the desc ctx
appears at a sufficiently aligned offset, and tweak the code so that all
calls to crypto_cipher_encrypt_one() operate on this buffer exclusively.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
crypto/ccm.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/crypto/ccm.c b/crypto/ccm.c
index 52e307807ff6..24c26ab052ca 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -58,7 +58,6 @@ struct cbcmac_tfm_ctx {
struct cbcmac_desc_ctx {
unsigned int len;
- u8 dg[];
};
static inline struct crypto_ccm_req_priv_ctx *crypto_ccm_reqctx(
@@ -868,9 +867,10 @@ static int crypto_cbcmac_digest_init(struct shash_desc *pdesc)
{
struct cbcmac_desc_ctx *ctx = shash_desc_ctx(pdesc);
int bs = crypto_shash_digestsize(pdesc->tfm);
+ u8 *dg = (u8 *)ctx + crypto_shash_descsize(pdesc->tfm) - bs;
ctx->len = 0;
- memset(ctx->dg, 0, bs);
+ memset(dg, 0, bs);
return 0;
}
@@ -883,17 +883,18 @@ static int crypto_cbcmac_digest_update(struct shash_desc *pdesc, const u8 *p,
struct cbcmac_desc_ctx *ctx = shash_desc_ctx(pdesc);
struct crypto_cipher *tfm = tctx->child;
int bs = crypto_shash_digestsize(parent);
+ u8 *dg = (u8 *)ctx + crypto_shash_descsize(parent) - bs;
while (len > 0) {
unsigned int l = min(len, bs - ctx->len);
- crypto_xor(ctx->dg + ctx->len, p, l);
+ crypto_xor(dg + ctx->len, p, l);
ctx->len +=l;
len -= l;
p += l;
if (ctx->len == bs) {
- crypto_cipher_encrypt_one(tfm, ctx->dg, ctx->dg);
+ crypto_cipher_encrypt_one(tfm, dg, dg);
ctx->len = 0;
}
}
@@ -908,12 +909,12 @@ static int crypto_cbcmac_digest_final(struct shash_desc *pdesc, u8 *out)
struct cbcmac_desc_ctx *ctx = shash_desc_ctx(pdesc);
struct crypto_cipher *tfm = tctx->child;
int bs = crypto_shash_digestsize(parent);
+ u8 *dg = (u8 *)ctx + crypto_shash_descsize(parent) - bs;
if (ctx->len)
- crypto_cipher_encrypt_one(tfm, out, ctx->dg);
- else
- memcpy(out, ctx->dg, bs);
+ crypto_cipher_encrypt_one(tfm, dg, dg);
+ memcpy(out, dg, bs);
return 0;
}
@@ -969,7 +970,8 @@ static int cbcmac_create(struct crypto_template *tmpl, struct rtattr **tb)
inst->alg.base.cra_blocksize = 1;
inst->alg.digestsize = alg->cra_blocksize;
- inst->alg.descsize = sizeof(struct cbcmac_desc_ctx) +
+ inst->alg.descsize = ALIGN(sizeof(struct cbcmac_desc_ctx),
+ alg->cra_alignmask + 1) +
alg->cra_blocksize;
inst->alg.base.cra_ctxsize = sizeof(struct cbcmac_tfm_ctx);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] crypto: arm64/crc32 - merge CRC32 and PMULL instruction based drivers
From: Herbert Xu @ 2017-02-11 10:51 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-arm-kernel, linux-crypto, will.deacon, yazen.ghannam,
steve.capper, agraf, mbrugger
In-Reply-To: <1485963340-25297-1-git-send-email-ard.biesheuvel@linaro.org>
On Wed, Feb 01, 2017 at 03:35:40PM +0000, Ard Biesheuvel wrote:
> The PMULL based CRC32 implementation already contains code based on the
> separate, optional CRC32 instructions to fallback to when operating on
> small quantities of data. We can expose these routines directly on systems
> that lack the 64x64 PMULL instructions but do implement the CRC32 ones,
> which makes the driver that is based solely on those CRC32 instructions
> redundant. So remove it.
>
> Note that this aligns arm64 with ARM, whose accelerated CRC32 driver
> also combines the CRC32 extension based and the PMULL based versions.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] crypto: generic/aes - drop alignment requirement
From: Herbert Xu @ 2017-02-11 10:52 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-crypto, ebiggers3
In-Reply-To: <1486051137-20511-1-git-send-email-ard.biesheuvel@linaro.org>
On Thu, Feb 02, 2017 at 03:58:57PM +0000, Ard Biesheuvel wrote:
> The generic AES code exposes a 32-bit align mask, which forces all
> users of the code to use temporary buffers or take other measures to
> ensure the alignment requirement is adhered to, even on architectures
> that don't care about alignment for software algorithms such as this
> one.
>
> So drop the align mask, and fix the code to use get_unaligned_le32()
> where appropriate, which will resolve to whatever is optimal for the
> architecture.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v3] crypto: aes - add generic time invariant AES cipher
From: Herbert Xu @ 2017-02-11 10:52 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-crypto, ebiggers3
In-Reply-To: <1486053460-7526-1-git-send-email-ard.biesheuvel@linaro.org>
On Thu, Feb 02, 2017 at 04:37:40PM +0000, Ard Biesheuvel wrote:
> Lookup table based AES is sensitive to timing attacks, which is due to
> the fact that such table lookups are data dependent, and the fact that
> 8 KB worth of tables covers a significant number of cachelines on any
> architecture, resulting in an exploitable correlation between the key
> and the processing time for known plaintexts.
>
> For network facing algorithms such as CTR, CCM or GCM, this presents a
> security risk, which is why arch specific AES ports are typically time
> invariant, either through the use of special instructions, or by using
> SIMD algorithms that don't rely on table lookups.
>
> For generic code, this is difficult to achieve without losing too much
> performance, but we can improve the situation significantly by switching
> to an implementation that only needs 256 bytes of table data (the actual
> S-box itself), which can be prefetched at the start of each block to
> eliminate data dependent latencies.
>
> This code encrypts at ~25 cycles per byte on ARM Cortex-A57 (while the
> ordinary generic AES driver manages 18 cycles per byte on this
> hardware). Decryption is substantially slower.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v3 0/3] crypto: time invariant AES for CCM (and CMAC/XCBC)
From: Herbert Xu @ 2017-02-11 10:53 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-crypto, ebiggers3, linux-arm-kernel
In-Reply-To: <1486133377-23290-1-git-send-email-ard.biesheuvel@linaro.org>
On Fri, Feb 03, 2017 at 02:49:34PM +0000, Ard Biesheuvel wrote:
> This series is primarily directed at improving the performance and security
> of CCM on the Rasperry Pi 3. This involves splitting the MAC handling of
> CCM into a separate driver so that we can efficiently replace it by something
> else using the ordinary algo resolution machinery.
>
> Patch #1 adds some testcases for cbcmac(aes), which will be introduced later.
>
> Patch #2 replaces the open coded CBC MAC hashing routines in the CCM driver
> with calls to a cbcmac() hash, and implements a template for producing such
> cbcmac transforms. This eliminates all the fuzzy scatterwalk code as well.
>
> Patch #3 implements cbcmac(aes) using NEON on arm64, and CMAC/XCBC at the
> same time, since it is trivially implemented reusing the same core transform
All applied. Please send any fixups on top of these patches.
Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v3] crypto: algapi - make crypto_xor() and crypto_inc() alignment agnostic
From: Herbert Xu @ 2017-02-11 10:55 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-crypto, ebiggers3, Jason
In-Reply-To: <1486289172-18604-1-git-send-email-ard.biesheuvel@linaro.org>
On Sun, Feb 05, 2017 at 10:06:12AM +0000, Ard Biesheuvel wrote:
> Instead of unconditionally forcing 4 byte alignment for all generic
> chaining modes that rely on crypto_xor() or crypto_inc() (which may
> result in unnecessary copying of data when the underlying hardware
> can perform unaligned accesses efficiently), make those functions
> deal with unaligned input explicitly, but only if the Kconfig symbol
> HAVE_EFFICIENT_UNALIGNED_ACCESS is set. This will allow us to drop
> the alignmasks from the CBC, CMAC, CTR, CTS, PCBC and SEQIV drivers.
>
> For crypto_inc(), this simply involves making the 4-byte stride
> conditional on HAVE_EFFICIENT_UNALIGNED_ACCESS being set, given that
> it typically operates on 16 byte buffers.
>
> For crypto_xor(), an algorithm is implemented that simply runs through
> the input using the largest strides possible if unaligned accesses are
> allowed. If they are not, an optimal sequence of memory accesses is
> emitted that takes the relative alignment of the input buffers into
> account, e.g., if the relative misalignment of dst and src is 4 bytes,
> the entire xor operation will be completed using 4 byte loads and stores
> (modulo unaligned bits at the start and end). Note that all expressions
> involving misalign are simply eliminated by the compiler when
> HAVE_EFFICIENT_UNALIGNED_ACCESS is defined.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: crypto: NULL deref in sha512_mb_mgr_get_comp_job_avx2
From: Herbert Xu @ 2017-02-11 10:50 UTC (permalink / raw)
To: Tim Chen
Cc: Dmitry Vyukov, David Miller, linux-crypto, LKML, megha.dey,
fenghua.yu, syzkaller
In-Reply-To: <20170201184502.GA11649@linux.intel.com>
On Wed, Feb 01, 2017 at 10:45:02AM -0800, Tim Chen wrote:
>
> One theory that Mehga and I have is that perhaps the flusher
> and regular computaion updates are stepping on each other.
> Can you try this patch and see if it helps?
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v6 0/3] Add Support for Cavium Cryptographic Acceleration Unit
From: Herbert Xu @ 2017-02-11 10:57 UTC (permalink / raw)
To: George Cherian
Cc: davem, david.daney, clabbe.montjoie, smueller, levinsasha928,
linux-kernel, linux-crypto
In-Reply-To: <1486479075-1813-1-git-send-email-george.cherian@cavium.com>
On Tue, Feb 07, 2017 at 02:51:12PM +0000, George Cherian wrote:
> This series adds the support for Cavium Cryptographic Accelerarion Unit (CPT)
> CPT is available in Cavium's Octeon-Tx SoC series.
>
> The series was tested with ecryptfs and dm-crypt for in kernel cryptographic
> offload operations. This driver needs a firmware to work, I will be sending the
> firmware to linux-firmware once the driver is accepted.
>
> Cahnges v5 -> v6
> -- Addressed Sasha Levin's comments.
> - Remove open coding of hlist_for_each_entry_safe, instead use
> the same.
> - Remove the direct access of hlist members
> - Remove unwanted argument checks.
All applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] crypto: improve gcc optimization flags for serpent and wp512
From: Herbert Xu @ 2017-02-11 10:54 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David S. Miller, kernel-build-reports, Ralf Baechle, linux-crypto,
linux-kernel
In-Reply-To: <20170203223334.3888507-1-arnd@arndb.de>
On Fri, Feb 03, 2017 at 11:33:23PM +0100, Arnd Bergmann wrote:
> An ancient gcc bug (first reported in 2003) has apparently resurfaced
> on MIPS, where kernelci.org reports an overly large stack frame in the
> whirlpool hash algorithm:
>
> crypto/wp512.c:987:1: warning: the frame size of 1112 bytes is larger than 1024 bytes [-Wframe-larger-than=]
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v3 0/3] crypto: time invariant AES for CCM (and CMAC/XCBC)
From: Ard Biesheuvel @ 2017-02-11 18:05 UTC (permalink / raw)
To: Herbert Xu
Cc: linux-crypto@vger.kernel.org, Eric Biggers,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20170211105339.GE855@gondor.apana.org.au>
On 11 February 2017 at 10:53, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Fri, Feb 03, 2017 at 02:49:34PM +0000, Ard Biesheuvel wrote:
>> This series is primarily directed at improving the performance and security
>> of CCM on the Rasperry Pi 3. This involves splitting the MAC handling of
>> CCM into a separate driver so that we can efficiently replace it by something
>> else using the ordinary algo resolution machinery.
>>
>> Patch #1 adds some testcases for cbcmac(aes), which will be introduced later.
>>
>> Patch #2 replaces the open coded CBC MAC hashing routines in the CCM driver
>> with calls to a cbcmac() hash, and implements a template for producing such
>> cbcmac transforms. This eliminates all the fuzzy scatterwalk code as well.
>>
>> Patch #3 implements cbcmac(aes) using NEON on arm64, and CMAC/XCBC at the
>> same time, since it is trivially implemented reusing the same core transform
>
> All applied. Please send any fixups on top of these patches.
Thanks Herbert. I do have a fixup for #2, which currently does not
correctly take the alignmask of the MAC's subordinate cipher into
account. I will send a fix for that shortly.
^ permalink raw reply
* Re: [PATCH] hwrng: cavium: Use per device name to allow for multiple devices.
From: Herbert Xu @ 2017-02-11 10:56 UTC (permalink / raw)
To: David Daney
Cc: linux-crypto, Matt Mackall, linux-kernel, Omer Khaliq,
Ananth Jasty
In-Reply-To: <20170206222846.22493-1-david.daney@cavium.com>
On Mon, Feb 06, 2017 at 02:28:46PM -0800, David Daney wrote:
> Systems containing the Cavium HW RNG may have one device per NUMA
> node. A typical configuration is a 2-node NUMA system, which results
> in 2 RNG devices. The hwrng subsystem refuses (and rightly so) to
> register more than one device with he same name, so we get failure
> messages on these systems.
>
> Make the hwrng name unique by including the underlying device name.
> Also remove spaces from the name to make it possible to switch devices
> via the sysfs knobs.
>
> Signed-off-by: David Daney <david.daney@cavium.com>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: linux-next: build warnings after merge of the crypto tree
From: Herbert Xu @ 2017-02-11 10:56 UTC (permalink / raw)
To: Stephen Rothwell
Cc: linux-next, linux-kernel, Cyrille Pitchen,
Linux Crypto Mailing List
In-Reply-To: <20170210141251.03b39b40@canb.auug.org.au>
On Fri, Feb 10, 2017 at 02:12:51PM +1100, Stephen Rothwell wrote:
>
> I am still getting these warnings ... I have seen no updates to the
> crypot tree since Feb 2.
Sorry Stephen. I have now applied Arnd's fixes for this problem
and it should be pushed out.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 1/2] crypto: atmel - refine Kconfig dependencies
From: Herbert Xu @ 2017-02-11 10:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Cyrille Pitchen, David S. Miller, linux-crypto, linux-kernel
In-Reply-To: <20170206123240.1698839-1-arnd@arndb.de>
On Mon, Feb 06, 2017 at 01:32:15PM +0100, Arnd Bergmann wrote:
> With the new authenc support, we get a harmless Kconfig warning:
>
> warning: (CRYPTO_DEV_ATMEL_AUTHENC) selects CRYPTO_DEV_ATMEL_SHA which has unmet direct dependencies (CRYPTO && CRYPTO_HW && ARCH_AT91)
>
> The problem is that each of the options has slightly different dependencies,
> although they all seem to want the same thing: allow building for real AT91
> targets that actually have the hardware, and possibly for compile testing.
>
> This makes all four options consistent: instead of depending on a particular
> dmaengine implementation, we depend on the ARM platform, CONFIG_COMPILE_TEST
> as an alternative when that is turned off. This makes the 'select' statements
> work correctly.
>
> Fixes: 89a82ef87e01 ("crypto: atmel-authenc - add support to authenc(hmac(shaX), Y(aes)) modes")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Both patches applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox