Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1621789 > unrolled thread

Re: [PATCH] Revert "arm64: Increase the max granular size"

Started byImran Khan <kimran@codeaurora.org>
First post2017-04-12 07:20 +0200
Last post2017-04-18 20:30 +0200
Articles 9 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  Re: [PATCH] Revert "arm64: Increase the max granular size" Imran Khan <kimran@codeaurora.org> - 2017-04-12 07:20 +0200
    Re: [PATCH] Revert "arm64: Increase the max granular size" "Chalamarla, Tirumalesh" <Tirumalesh.Chalamarla@cavium.com> - 2017-04-12 16:10 +0200
      Re: [PATCH] Revert "arm64: Increase the max granular size" Imran Khan <kimran@codeaurora.org> - 2017-04-17 09:40 +0200
        Re: [PATCH] Revert "arm64: Increase the max granular size" Sunil Kovvuri <sunil.kovvuri@gmail.com> - 2017-04-17 12:40 +0200
          Re: [PATCH] Revert "arm64: Increase the max granular size" Catalin Marinas <catalin.marinas@arm.com> - 2017-04-18 16:50 +0200
            Re: [PATCH] Revert "arm64: Increase the max granular size" Sunil Kovvuri <sunil.kovvuri@gmail.com> - 2017-04-18 19:10 +0200
              Re: [PATCH] Revert "arm64: Increase the max granular size" Catalin Marinas <catalin.marinas@arm.com> - 2017-04-19 14:10 +0200
                Re: [PATCH] Revert "arm64: Increase the max granular size" Sunil Kovvuri <sunil.kovvuri@gmail.com> - 2017-04-19 15:20 +0200
        Re: [PATCH] Revert "arm64: Increase the max granular size" "Chalamarla, Tirumalesh" <Tirumalesh.Chalamarla@cavium.com> - 2017-04-18 20:30 +0200

#1621789 — Re: [PATCH] Revert "arm64: Increase the max granular size"

FromImran Khan <kimran@codeaurora.org>
Date2017-04-12 07:20 +0200
SubjectRe: [PATCH] Revert "arm64: Increase the max granular size"
Message-ID<tviRb-rj-9@gated-at.bofh.it>
On 4/7/2017 7:36 AM, Ganesh Mahendran wrote:
> 2017-04-06 23:58 GMT+08:00 Catalin Marinas <catalin.marinas@arm.com>:
>> On Thu, Apr 06, 2017 at 12:52:13PM +0530, Imran Khan wrote:
>>> On 4/5/2017 10:13 AM, Imran Khan wrote:
>>>>> We may have to revisit this logic and consider L1_CACHE_BYTES the
>>>>> _minimum_ of cache line sizes in arm64 systems supported by the kernel.
>>>>> Do you have any benchmarks on Cavium boards that would show significant
>>>>> degradation with 64-byte L1_CACHE_BYTES vs 128?
>>>>>
>>>>> For non-coherent DMA, the simplest is to make ARCH_DMA_MINALIGN the
>>>>> _maximum_ of the supported systems:
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
>>>>> index 5082b30bc2c0..4b5d7b27edaf 100644
>>>>> --- a/arch/arm64/include/asm/cache.h
>>>>> +++ b/arch/arm64/include/asm/cache.h
>>>>> @@ -18,17 +18,17 @@
>>>>>
>>>>>  #include <asm/cachetype.h>
>>>>>
>>>>> -#define L1_CACHE_SHIFT         7
>>>>> +#define L1_CACHE_SHIFT         6
>>>>>  #define L1_CACHE_BYTES         (1 << L1_CACHE_SHIFT)
>>>>>
>>>>>  /*
>>>>>   * Memory returned by kmalloc() may be used for DMA, so we must make
>>>>> - * sure that all such allocations are cache aligned. Otherwise,
>>>>> - * unrelated code may cause parts of the buffer to be read into the
>>>>> - * cache before the transfer is done, causing old data to be seen by
>>>>> - * the CPU.
>>>>> + * sure that all such allocations are aligned to the maximum *known*
>>>>> + * cache line size on ARMv8 systems. Otherwise, unrelated code may cause
>>>>> + * parts of the buffer to be read into the cache before the transfer is
>>>>> + * done, causing old data to be seen by the CPU.
>>>>>   */
>>>>> -#define ARCH_DMA_MINALIGN      L1_CACHE_BYTES
>>>>> +#define ARCH_DMA_MINALIGN      (128)
>>>>>
>>>>>  #ifndef __ASSEMBLY__
>>>>>
>>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>>> index 392c67eb9fa6..30bafca1aebf 100644
>>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>>> @@ -976,9 +976,9 @@ void __init setup_cpu_features(void)
>>>>>         if (!cwg)
>>>>>                 pr_warn("No Cache Writeback Granule information, assuming
>>>>> cache line size %d\n",
>>>>>                         cls);
>>>>> -       if (L1_CACHE_BYTES < cls)
>>>>> -               pr_warn("L1_CACHE_BYTES smaller than the Cache Writeback Granule (%d < %d)\n",
>>>>> -                       L1_CACHE_BYTES, cls);
>>>>> +       if (ARCH_DMA_MINALIGN < cls)
>>>>> +               pr_warn("ARCH_DMA_MINALIGN smaller than the Cache Writeback Granule (%d < %d)\n",
>>>>> +                       ARCH_DMA_MINALIGN, cls);
>>>>>  }
>>>>>
>>>>>  static bool __maybe_unused
>>>>
>>>> This change was discussed at: [1] but was not concluded as apparently no one
>>>> came back with test report and numbers. After including this change in our
>>>> local kernel we are seeing significant throughput improvement. For example with:
>>>>
>>>> iperf -c 192.168.1.181 -i 1 -w 128K -t 60
>>>>
>>>> The average throughput is improving by about 30% (230Mbps from 180Mbps).
>>>> Could you please let us know if this change can be included in upstream kernel.
>>>>
>>>> [1]: https://groups.google.com/forum/#!topic/linux.kernel/P40yDB90ePs
>>>
>>> Could you please provide some feedback about the above mentioned query ?
>>
>> Do you have an explanation on the performance variation when
>> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
>> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
>> non-coherent DMA?).
> 
> network stack use SKB_DATA_ALIGN to align.
> ---
> #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
> ~(SMP_CACHE_BYTES - 1))
> 
> #define SMP_CACHE_BYTES L1_CACHE_BYTES
> ---
> I think this is the reason of performance regression.
> 

Yes this is the reason for performance regression. Due to increases L1 cache alignment the 
object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to 
4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.

>>
>> The Cavium guys haven't shown any numbers (IIUC) to back the
>> L1_CACHE_BYTES performance improvement but I would not revert the
>> original commit since ARCH_DMA_MINALIGN definitely needs to cover the
>> maximum available cache line size, which is 128 for them.
> 
> how about define L1_CACHE_SHIFT like below:
> ---
> #ifdef CONFIG_ARM64_L1_CACHE_SHIFT
> #define L1_CACHE_SHIFT CONFIG_ARM64_L1_CACHE_SHIFT
> #else
> #define L1_CACHE_SHIFT 7
> endif
> ---
> 
> Thanks
> 
>>
>> --
>> Catalin


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation

[toc] | [next] | [standalone]


#1622162

From"Chalamarla, Tirumalesh" <Tirumalesh.Chalamarla@cavium.com>
Date2017-04-12 16:10 +0200
Message-ID<tvr86-5qV-33@gated-at.bofh.it>
In reply to#1621789

On 4/11/17, 10:13 PM, "linux-arm-kernel on behalf of Imran Khan" <linux-arm-kernel-bounces@lists.infradead.org on behalf of kimran@codeaurora.org> wrote:

    On 4/7/2017 7:36 AM, Ganesh Mahendran wrote:
    > 2017-04-06 23:58 GMT+08:00 Catalin Marinas <catalin.marinas@arm.com>:
    >> On Thu, Apr 06, 2017 at 12:52:13PM +0530, Imran Khan wrote:
    >>> On 4/5/2017 10:13 AM, Imran Khan wrote:
    >>>>> We may have to revisit this logic and consider L1_CACHE_BYTES the
    >>>>> _minimum_ of cache line sizes in arm64 systems supported by the kernel.
    >>>>> Do you have any benchmarks on Cavium boards that would show significant
    >>>>> degradation with 64-byte L1_CACHE_BYTES vs 128?
    >>>>>
    >>>>> For non-coherent DMA, the simplest is to make ARCH_DMA_MINALIGN the
    >>>>> _maximum_ of the supported systems:
    >>>>>
    >>>>> diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
    >>>>> index 5082b30bc2c0..4b5d7b27edaf 100644
    >>>>> --- a/arch/arm64/include/asm/cache.h
    >>>>> +++ b/arch/arm64/include/asm/cache.h
    >>>>> @@ -18,17 +18,17 @@
    >>>>>
    >>>>>  #include <asm/cachetype.h>
    >>>>>
    >>>>> -#define L1_CACHE_SHIFT         7
    >>>>> +#define L1_CACHE_SHIFT         6
    >>>>>  #define L1_CACHE_BYTES         (1 << L1_CACHE_SHIFT)
    >>>>>
    >>>>>  /*
    >>>>>   * Memory returned by kmalloc() may be used for DMA, so we must make
    >>>>> - * sure that all such allocations are cache aligned. Otherwise,
    >>>>> - * unrelated code may cause parts of the buffer to be read into the
    >>>>> - * cache before the transfer is done, causing old data to be seen by
    >>>>> - * the CPU.
    >>>>> + * sure that all such allocations are aligned to the maximum *known*
    >>>>> + * cache line size on ARMv8 systems. Otherwise, unrelated code may cause
    >>>>> + * parts of the buffer to be read into the cache before the transfer is
    >>>>> + * done, causing old data to be seen by the CPU.
    >>>>>   */
    >>>>> -#define ARCH_DMA_MINALIGN      L1_CACHE_BYTES
    >>>>> +#define ARCH_DMA_MINALIGN      (128)
    >>>>>
    >>>>>  #ifndef __ASSEMBLY__
    >>>>>
    >>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
    >>>>> index 392c67eb9fa6..30bafca1aebf 100644
    >>>>> --- a/arch/arm64/kernel/cpufeature.c
    >>>>> +++ b/arch/arm64/kernel/cpufeature.c
    >>>>> @@ -976,9 +976,9 @@ void __init setup_cpu_features(void)
    >>>>>         if (!cwg)
    >>>>>                 pr_warn("No Cache Writeback Granule information, assuming
    >>>>> cache line size %d\n",
    >>>>>                         cls);
    >>>>> -       if (L1_CACHE_BYTES < cls)
    >>>>> -               pr_warn("L1_CACHE_BYTES smaller than the Cache Writeback Granule (%d < %d)\n",
    >>>>> -                       L1_CACHE_BYTES, cls);
    >>>>> +       if (ARCH_DMA_MINALIGN < cls)
    >>>>> +               pr_warn("ARCH_DMA_MINALIGN smaller than the Cache Writeback Granule (%d < %d)\n",
    >>>>> +                       ARCH_DMA_MINALIGN, cls);
    >>>>>  }
    >>>>>
    >>>>>  static bool __maybe_unused
    >>>>
    >>>> This change was discussed at: [1] but was not concluded as apparently no one
    >>>> came back with test report and numbers. After including this change in our
    >>>> local kernel we are seeing significant throughput improvement. For example with:
    >>>>
    >>>> iperf -c 192.168.1.181 -i 1 -w 128K -t 60
    >>>>
    >>>> The average throughput is improving by about 30% (230Mbps from 180Mbps).
    >>>> Could you please let us know if this change can be included in upstream kernel.
    >>>>
    >>>> [1]: https://groups.google.com/forum/#!topic/linux.kernel/P40yDB90ePs
    >>>
    >>> Could you please provide some feedback about the above mentioned query ?
    >>
    >> Do you have an explanation on the performance variation when
    >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
    >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
    >> non-coherent DMA?).
    > 
    > network stack use SKB_DATA_ALIGN to align.
    > ---
    > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
    > ~(SMP_CACHE_BYTES - 1))
    > 
    > #define SMP_CACHE_BYTES L1_CACHE_BYTES
    > ---
    > I think this is the reason of performance regression.
    > 
    
    Yes this is the reason for performance regression. Due to increases L1 cache alignment the 
    object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to 
    4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.
    
We tried different benchmarks and found none which really affects with Cache line change. If there is no correctness issue,
I think we are fine with reverting the patch.

Though I still think it is beneficiary to do some more investigation for the perf loss, who knows 32 bit align or no align might 
Give even more perf benefit. 


Thanks,
Tirumalesh.  
    >>
    >> The Cavium guys haven't shown any numbers (IIUC) to back the
    >> L1_CACHE_BYTES performance improvement but I would not revert the
    >> original commit since ARCH_DMA_MINALIGN definitely needs to cover the
    >> maximum available cache line size, which is 128 for them.
    > 
    > how about define L1_CACHE_SHIFT like below:
    > ---
    > #ifdef CONFIG_ARM64_L1_CACHE_SHIFT
    > #define L1_CACHE_SHIFT CONFIG_ARM64_L1_CACHE_SHIFT
    > #else
    > #define L1_CACHE_SHIFT 7
    > endif
    > ---
    > 
    > Thanks
    > 
    >>
    >> --
    >> Catalin
    
    
    -- 
    QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
    
    _______________________________________________
    linux-arm-kernel mailing list
    linux-arm-kernel@lists.infradead.org
    http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
    

[toc] | [prev] | [next] | [standalone]


#1624583

FromImran Khan <kimran@codeaurora.org>
Date2017-04-17 09:40 +0200
Message-ID<tx9qq-6lX-11@gated-at.bofh.it>
In reply to#1622162
On 4/12/2017 7:30 PM, Chalamarla, Tirumalesh wrote:
> 
> 
> On 4/11/17, 10:13 PM, "linux-arm-kernel on behalf of Imran Khan" <linux-arm-kernel-bounces@lists.infradead.org on behalf of kimran@codeaurora.org> wrote:
> 
>     On 4/7/2017 7:36 AM, Ganesh Mahendran wrote:
>     > 2017-04-06 23:58 GMT+08:00 Catalin Marinas <catalin.marinas@arm.com>:
>     >> On Thu, Apr 06, 2017 at 12:52:13PM +0530, Imran Khan wrote:
>     >>> On 4/5/2017 10:13 AM, Imran Khan wrote:
>     >>>>> We may have to revisit this logic and consider L1_CACHE_BYTES the
>     >>>>> _minimum_ of cache line sizes in arm64 systems supported by the kernel.
>     >>>>> Do you have any benchmarks on Cavium boards that would show significant
>     >>>>> degradation with 64-byte L1_CACHE_BYTES vs 128?
>     >>>>>
>     >>>>> For non-coherent DMA, the simplest is to make ARCH_DMA_MINALIGN the
>     >>>>> _maximum_ of the supported systems:
>     >>>>>
>     >>>>> diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
>     >>>>> index 5082b30bc2c0..4b5d7b27edaf 100644
>     >>>>> --- a/arch/arm64/include/asm/cache.h
>     >>>>> +++ b/arch/arm64/include/asm/cache.h
>     >>>>> @@ -18,17 +18,17 @@
>     >>>>>
>     >>>>>  #include <asm/cachetype.h>
>     >>>>>
>     >>>>> -#define L1_CACHE_SHIFT         7
>     >>>>> +#define L1_CACHE_SHIFT         6
>     >>>>>  #define L1_CACHE_BYTES         (1 << L1_CACHE_SHIFT)
>     >>>>>
>     >>>>>  /*
>     >>>>>   * Memory returned by kmalloc() may be used for DMA, so we must make
>     >>>>> - * sure that all such allocations are cache aligned. Otherwise,
>     >>>>> - * unrelated code may cause parts of the buffer to be read into the
>     >>>>> - * cache before the transfer is done, causing old data to be seen by
>     >>>>> - * the CPU.
>     >>>>> + * sure that all such allocations are aligned to the maximum *known*
>     >>>>> + * cache line size on ARMv8 systems. Otherwise, unrelated code may cause
>     >>>>> + * parts of the buffer to be read into the cache before the transfer is
>     >>>>> + * done, causing old data to be seen by the CPU.
>     >>>>>   */
>     >>>>> -#define ARCH_DMA_MINALIGN      L1_CACHE_BYTES
>     >>>>> +#define ARCH_DMA_MINALIGN      (128)
>     >>>>>
>     >>>>>  #ifndef __ASSEMBLY__
>     >>>>>
>     >>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>     >>>>> index 392c67eb9fa6..30bafca1aebf 100644
>     >>>>> --- a/arch/arm64/kernel/cpufeature.c
>     >>>>> +++ b/arch/arm64/kernel/cpufeature.c
>     >>>>> @@ -976,9 +976,9 @@ void __init setup_cpu_features(void)
>     >>>>>         if (!cwg)
>     >>>>>                 pr_warn("No Cache Writeback Granule information, assuming
>     >>>>> cache line size %d\n",
>     >>>>>                         cls);
>     >>>>> -       if (L1_CACHE_BYTES < cls)
>     >>>>> -               pr_warn("L1_CACHE_BYTES smaller than the Cache Writeback Granule (%d < %d)\n",
>     >>>>> -                       L1_CACHE_BYTES, cls);
>     >>>>> +       if (ARCH_DMA_MINALIGN < cls)
>     >>>>> +               pr_warn("ARCH_DMA_MINALIGN smaller than the Cache Writeback Granule (%d < %d)\n",
>     >>>>> +                       ARCH_DMA_MINALIGN, cls);
>     >>>>>  }
>     >>>>>
>     >>>>>  static bool __maybe_unused
>     >>>>
>     >>>> This change was discussed at: [1] but was not concluded as apparently no one
>     >>>> came back with test report and numbers. After including this change in our
>     >>>> local kernel we are seeing significant throughput improvement. For example with:
>     >>>>
>     >>>> iperf -c 192.168.1.181 -i 1 -w 128K -t 60
>     >>>>
>     >>>> The average throughput is improving by about 30% (230Mbps from 180Mbps).
>     >>>> Could you please let us know if this change can be included in upstream kernel.
>     >>>>
>     >>>> [1]: https://groups.google.com/forum/#!topic/linux.kernel/P40yDB90ePs
>     >>>
>     >>> Could you please provide some feedback about the above mentioned query ?
>     >>
>     >> Do you have an explanation on the performance variation when
>     >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
>     >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
>     >> non-coherent DMA?).
>     > 
>     > network stack use SKB_DATA_ALIGN to align.
>     > ---
>     > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
>     > ~(SMP_CACHE_BYTES - 1))
>     > 
>     > #define SMP_CACHE_BYTES L1_CACHE_BYTES
>     > ---
>     > I think this is the reason of performance regression.
>     > 
>     
>     Yes this is the reason for performance regression. Due to increases L1 cache alignment the 
>     object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to 
>     4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.
>     
> We tried different benchmarks and found none which really affects with Cache line change. If there is no correctness issue,
> I think we are fine with reverting the patch.
> 
So, can we revert the patch that makes L1_CACHE_SHIFT 7 or should the patch suggested by Catalin should be mainlined.
We have verified the throughput degradation on 3.18 and 4.4 but I am afraid that this issue will be seen on other
kernels too.
> Though I still think it is beneficiary to do some more investigation for the perf loss, who knows 32 bit align or no align might 
> Give even more perf benefit. 
> 
Which perf loss you are referring to here. Did you mean throughput loss here or some other perf benchmarking ?

Thanks,
Imran

> 
> Thanks,
> Tirumalesh.  
>     >>
>     >> The Cavium guys haven't shown any numbers (IIUC) to back the
>     >> L1_CACHE_BYTES performance improvement but I would not revert the
>     >> original commit since ARCH_DMA_MINALIGN definitely needs to cover the
>     >> maximum available cache line size, which is 128 for them.
>     > 
>     > how about define L1_CACHE_SHIFT like below:
>     > ---
>     > #ifdef CONFIG_ARM64_L1_CACHE_SHIFT
>     > #define L1_CACHE_SHIFT CONFIG_ARM64_L1_CACHE_SHIFT
>     > #else
>     > #define L1_CACHE_SHIFT 7
>     > endif
>     > ---
>     > 
>     > Thanks
>     > 
>     >>
>     >> --
>     >> Catalin
>     
>     
>     -- 
>     QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
>     
>     _______________________________________________
>     linux-arm-kernel mailing list
>     linux-arm-kernel@lists.infradead.org
>     http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>     
> 


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation

[toc] | [prev] | [next] | [standalone]


#1624634

FromSunil Kovvuri <sunil.kovvuri@gmail.com>
Date2017-04-17 12:40 +0200
Message-ID<txceC-7ZQ-15@gated-at.bofh.it>
In reply to#1624583
>>     >> Do you have an explanation on the performance variation when
>>     >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
>>     >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
>>     >> non-coherent DMA?).
>>     >
>>     > network stack use SKB_DATA_ALIGN to align.
>>     > ---
>>     > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
>>     > ~(SMP_CACHE_BYTES - 1))
>>     >
>>     > #define SMP_CACHE_BYTES L1_CACHE_BYTES
>>     > ---
>>     > I think this is the reason of performance regression.
>>     >
>>
>>     Yes this is the reason for performance regression. Due to increases L1 cache alignment the
>>     object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to
>>     4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.

With what traffic did you check 'skb->truesize' ?
Increase from 2304 to 4352 bytes doesn't seem to be real. I checked
with ICMP pkts with maximum
size possible with 1500byte MTU and I don't see such a bump. If the
bump is observed with Iperf
sending TCP packets then I suggest to check if TSO is playing a part over here.

And for 'sk_wmem_alloc', I have done Iperf benchmarking on a 40G
interface and I hit linerate irrespective
of cache line size being 64 or 128 bytes. I guess transmit completion
latency on your HW or driver is very
high and that seems to be the real issue for low performance and not
due to cache line size, basically you are
not able to freeup skbs/buffers fast enough so that new ones get queued up.

Doesn't skb_orphan() solve your issue ?
FYI,
https://patchwork.ozlabs.org/patch/455134/
http://lxr.free-electrons.com/source/drivers/net/ethernet/chelsio/cxgb3/sge.c#L1288


>>
>> We tried different benchmarks and found none which really affects with Cache line change. If there is no correctness issue,
>> I think we are fine with reverting the patch.
>>
> So, can we revert the patch that makes L1_CACHE_SHIFT 7 or should the patch suggested by Catalin should be mainlined.

This doesn't seem right, as someone said earlier what if there is
another arm64 platform with 32bytes
cacheline size and wants to reduce this further. Either this should be
made platform dependent or left as is
i.e that is maximum of all.

Thanks,
Sunil.

[toc] | [prev] | [next] | [standalone]


#1625373

FromCatalin Marinas <catalin.marinas@arm.com>
Date2017-04-18 16:50 +0200
Message-ID<txCC6-7kc-3@gated-at.bofh.it>
In reply to#1624634
On Mon, Apr 17, 2017 at 04:08:52PM +0530, Sunil Kovvuri wrote:
> >>     >> Do you have an explanation on the performance variation when
> >>     >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
> >>     >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
> >>     >> non-coherent DMA?).
> >>     >
> >>     > network stack use SKB_DATA_ALIGN to align.
> >>     > ---
> >>     > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
> >>     > ~(SMP_CACHE_BYTES - 1))
> >>     >
> >>     > #define SMP_CACHE_BYTES L1_CACHE_BYTES
> >>     > ---
> >>     > I think this is the reason of performance regression.
> >>     >
> >>
> >>     Yes this is the reason for performance regression. Due to increases L1 cache alignment the
> >>     object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to
> >>     4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.
> 
> With what traffic did you check 'skb->truesize' ?
> Increase from 2304 to 4352 bytes doesn't seem to be real. I checked
> with ICMP pkts with maximum
> size possible with 1500byte MTU and I don't see such a bump. If the
> bump is observed with Iperf
> sending TCP packets then I suggest to check if TSO is playing a part over here.

I haven't checked truesize but I added some printks to __alloc_skb() (on
a Juno platform) and the size argument to this function is 1720 on many
occasions. With sizeof(struct skb_shared_info) of 320, the actual data
allocation is exactly 2048 when using 64 byte L1_CACHE_SIZE. With a
128 byte cache size, it goes slightly over 2K, hence the 4K slab
allocation. The 1720 figure surprised me a bit as well since I was
expecting something close to 1500.

The thing that worries me is that skb->data may be used as a buffer to
DMA into. If that's the case, skb_shared_info is wrongly aligned based
on SMP_CACHE_BYTES only and can lead to corruption on a non-DMA-coherent
platform. It should really be ARCH_DMA_MINALIGN.

IIUC, the Cavium platform has coherent DMA, so it shouldn't be an issue
if we go back to 64 byte cache lines. However, we don't really have an
easy way to check (maybe taint the kernel if CWG is different from
ARCH_DMA_MINALIGN *and* the non-coherent DMA API is called).

-- 
Catalin

[toc] | [prev] | [next] | [standalone]


#1625474

FromSunil Kovvuri <sunil.kovvuri@gmail.com>
Date2017-04-18 19:10 +0200
Message-ID<txENB-le-25@gated-at.bofh.it>
In reply to#1625373
On Tue, Apr 18, 2017 at 8:18 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Mon, Apr 17, 2017 at 04:08:52PM +0530, Sunil Kovvuri wrote:
>> >>     >> Do you have an explanation on the performance variation when
>> >>     >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
>> >>     >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
>> >>     >> non-coherent DMA?).
>> >>     >
>> >>     > network stack use SKB_DATA_ALIGN to align.
>> >>     > ---
>> >>     > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
>> >>     > ~(SMP_CACHE_BYTES - 1))
>> >>     >
>> >>     > #define SMP_CACHE_BYTES L1_CACHE_BYTES
>> >>     > ---
>> >>     > I think this is the reason of performance regression.
>> >>     >
>> >>
>> >>     Yes this is the reason for performance regression. Due to increases L1 cache alignment the
>> >>     object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to
>> >>     4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.
>>
>> With what traffic did you check 'skb->truesize' ?
>> Increase from 2304 to 4352 bytes doesn't seem to be real. I checked
>> with ICMP pkts with maximum
>> size possible with 1500byte MTU and I don't see such a bump. If the
>> bump is observed with Iperf
>> sending TCP packets then I suggest to check if TSO is playing a part over here.
>
> I haven't checked truesize but I added some printks to __alloc_skb() (on
> a Juno platform) and the size argument to this function is 1720 on many
> occasions. With sizeof(struct skb_shared_info) of 320, the actual data
> allocation is exactly 2048 when using 64 byte L1_CACHE_SIZE. With a
> 128 byte cache size, it goes slightly over 2K, hence the 4K slab
> allocation.

Understood but still in my opinion this '4K slab allocation' cannot be
considered as
an issue with cache line size, there are many network drivers out
there which do
receive buffer or page recycling to minimize (sometimes almost to
zero) the cost
of buffer allocation.

>The 1720 figure surprised me a bit as well since I was
> expecting something close to 1500.
>
> The thing that worries me is that skb->data may be used as a buffer to
> DMA into. If that's the case, skb_shared_info is wrongly aligned based
> on SMP_CACHE_BYTES only and can lead to corruption on a non-DMA-coherent
> platform. It should really be ARCH_DMA_MINALIGN.

I didn't get this, if you see __alloc_skb()

229         size = SKB_DATA_ALIGN(size);
230         size += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

both DMA buffer and skb_shared_info are aligned to a cacheline separately,
considering 128byte alignment guarantees 64byte alignment as well, how
will this
lead to corruption ?

And if platform is non-DMA-coherent then again it's the driver which
should take
of coherency by using appropriate map/unmap APIs and should avoid any cacheline
sharing btw DMA buffer and skb_shared_info.

>
> IIUC, the Cavium platform has coherent DMA, so it shouldn't be an issue
> if we go back to 64 byte cache lines.

Yes, Cavium platform is DMA coherent and there is no issue with reverting back
to 64byte cachelines. But do we want to do this because some platform has a
performance issue and this is an easy way to solve it. IMHO there seems
to be many ways to solve performance degradation within the driver itself, and
if those doesn't work then probably it makes sense to revert this.

>However, we don't really have an
> easy way to check (maybe taint the kernel if CWG is different from
> ARCH_DMA_MINALIGN *and* the non-coherent DMA API is called).
>
> --
> Catalin

[toc] | [prev] | [next] | [standalone]


#1626052

FromCatalin Marinas <catalin.marinas@arm.com>
Date2017-04-19 14:10 +0200
Message-ID<txWAO-3iG-25@gated-at.bofh.it>
In reply to#1625474
On Tue, Apr 18, 2017 at 10:35:02PM +0530, Sunil Kovvuri wrote:
> On Tue, Apr 18, 2017 at 8:18 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Mon, Apr 17, 2017 at 04:08:52PM +0530, Sunil Kovvuri wrote:
> >> >>     >> Do you have an explanation on the performance variation when
> >> >>     >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
> >> >>     >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
> >> >>     >> non-coherent DMA?).
> >> >>     >
> >> >>     > network stack use SKB_DATA_ALIGN to align.
> >> >>     > ---
> >> >>     > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
> >> >>     > ~(SMP_CACHE_BYTES - 1))
> >> >>     >
> >> >>     > #define SMP_CACHE_BYTES L1_CACHE_BYTES
> >> >>     > ---
> >> >>     > I think this is the reason of performance regression.
> >> >>     >
> >> >>
> >> >>     Yes this is the reason for performance regression. Due to increases L1 cache alignment the
> >> >>     object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to
> >> >>     4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.
> >>
> >> With what traffic did you check 'skb->truesize' ? Increase from
> >> 2304 to 4352 bytes doesn't seem to be real. I checked with ICMP
> >> pkts with maximum size possible with 1500byte MTU and I don't see
> >> such a bump. If the bump is observed with Iperf sending TCP packets
> >> then I suggest to check if TSO is playing a part over here.
> >
> > I haven't checked truesize but I added some printks to __alloc_skb() (on
> > a Juno platform) and the size argument to this function is 1720 on many
> > occasions. With sizeof(struct skb_shared_info) of 320, the actual data
> > allocation is exactly 2048 when using 64 byte L1_CACHE_SIZE. With a
> > 128 byte cache size, it goes slightly over 2K, hence the 4K slab
> > allocation.
> 
> Understood but still in my opinion this '4K slab allocation' cannot be
> considered as an issue with cache line size, there are many network
> drivers out there which do receive buffer or page recycling to
> minimize (sometimes almost to zero) the cost of buffer allocation.

The slab allocation shouldn't make much difference (unless you are
running on a memory constrained system) but I don't understand how
skb->truesize (which is almost half unused) affects the sk_wmem_alloc
and its interaction with other bits in the network stack (e.g.
tcp_limit_output_bytes).

However, I do think it's worth investigating further to fully understand
the issue.

> >The 1720 figure surprised me a bit as well since I was
> > expecting something close to 1500.
> >
> > The thing that worries me is that skb->data may be used as a buffer to
> > DMA into. If that's the case, skb_shared_info is wrongly aligned based
> > on SMP_CACHE_BYTES only and can lead to corruption on a non-DMA-coherent
> > platform. It should really be ARCH_DMA_MINALIGN.
> 
> I didn't get this, if you see __alloc_skb()
> 
> 229         size = SKB_DATA_ALIGN(size);
> 230         size += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> 
> both DMA buffer and skb_shared_info are aligned to a cacheline separately,
> considering 128byte alignment guarantees 64byte alignment as well, how
> will this lead to corruption ?

It's the other way around: you align only to 64 byte while running on a
platform with 128 byte cache lines and non-coherent DMA.

> And if platform is non-DMA-coherent then again it's the driver which
> should take of coherency by using appropriate map/unmap APIs and
> should avoid any cacheline sharing btw DMA buffer and skb_shared_info.

The problem is that the streaming DMA API can only work correctly on
cacheline-aligned buffers (because of the cache invalidation it performs
for DMA ops; even with clean&invalidate, the operation isn't always safe
if a cacheline is shared between DMA and CPU buffers). In the skb case,
we could have the data potentially sharing the last addresses of a DMA
buffer with struct skb_shared_info.

We may be able to get away with SKB_DATA_ALIGN not using
ARCH_DMA_MINALIGN *if* skb_shared_info is *not* written before or during
an inbound DMA transfer (though such tricks are arch specific).

> > IIUC, the Cavium platform has coherent DMA, so it shouldn't be an issue
> > if we go back to 64 byte cache lines.
> 
> Yes, Cavium platform is DMA coherent and there is no issue with reverting back
> to 64byte cachelines. But do we want to do this because some platform has a
> performance issue and this is an easy way to solve it. IMHO there seems
> to be many ways to solve performance degradation within the driver itself, and
> if those doesn't work then probably it makes sense to revert this.

My initial thought was to revert the change because it was causing a
significant performance regression on certain SoC. But given that it
took over a year for people to follow up, it doesn't seem too urgent, so
we should rather try to understand the issue and potential side effects
of moving back to a 64 byte cache line.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]


#1626107

FromSunil Kovvuri <sunil.kovvuri@gmail.com>
Date2017-04-19 15:20 +0200
Message-ID<txXGy-3U9-13@gated-at.bofh.it>
In reply to#1626052
On Wed, Apr 19, 2017 at 5:31 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Tue, Apr 18, 2017 at 10:35:02PM +0530, Sunil Kovvuri wrote:
>> On Tue, Apr 18, 2017 at 8:18 PM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > On Mon, Apr 17, 2017 at 04:08:52PM +0530, Sunil Kovvuri wrote:
>> >> >>     >> Do you have an explanation on the performance variation when
>> >> >>     >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
>> >> >>     >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
>> >> >>     >> non-coherent DMA?).
>> >> >>     >
>> >> >>     > network stack use SKB_DATA_ALIGN to align.
>> >> >>     > ---
>> >> >>     > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
>> >> >>     > ~(SMP_CACHE_BYTES - 1))
>> >> >>     >
>> >> >>     > #define SMP_CACHE_BYTES L1_CACHE_BYTES
>> >> >>     > ---
>> >> >>     > I think this is the reason of performance regression.
>> >> >>     >
>> >> >>
>> >> >>     Yes this is the reason for performance regression. Due to increases L1 cache alignment the
>> >> >>     object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to
>> >> >>     4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.
>> >>
>> >> With what traffic did you check 'skb->truesize' ? Increase from
>> >> 2304 to 4352 bytes doesn't seem to be real. I checked with ICMP
>> >> pkts with maximum size possible with 1500byte MTU and I don't see
>> >> such a bump. If the bump is observed with Iperf sending TCP packets
>> >> then I suggest to check if TSO is playing a part over here.
>> >
>> > I haven't checked truesize but I added some printks to __alloc_skb() (on
>> > a Juno platform) and the size argument to this function is 1720 on many
>> > occasions. With sizeof(struct skb_shared_info) of 320, the actual data
>> > allocation is exactly 2048 when using 64 byte L1_CACHE_SIZE. With a
>> > 128 byte cache size, it goes slightly over 2K, hence the 4K slab
>> > allocation.
>>
>> Understood but still in my opinion this '4K slab allocation' cannot be
>> considered as an issue with cache line size, there are many network
>> drivers out there which do receive buffer or page recycling to
>> minimize (sometimes almost to zero) the cost of buffer allocation.
>
> The slab allocation shouldn't make much difference (unless you are
> running on a memory constrained system) but I don't understand how
> skb->truesize (which is almost half unused) affects the sk_wmem_alloc
> and its interaction with other bits in the network stack (e.g.
> tcp_limit_output_bytes).
>
> However, I do think it's worth investigating further to fully understand
> the issue.

Absolutely.

>
>> >The 1720 figure surprised me a bit as well since I was
>> > expecting something close to 1500.
>> >
>> > The thing that worries me is that skb->data may be used as a buffer to
>> > DMA into. If that's the case, skb_shared_info is wrongly aligned based
>> > on SMP_CACHE_BYTES only and can lead to corruption on a non-DMA-coherent
>> > platform. It should really be ARCH_DMA_MINALIGN.
>>
>> I didn't get this, if you see __alloc_skb()
>>
>> 229         size = SKB_DATA_ALIGN(size);
>> 230         size += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>>
>> both DMA buffer and skb_shared_info are aligned to a cacheline separately,
>> considering 128byte alignment guarantees 64byte alignment as well, how
>> will this lead to corruption ?
>
> It's the other way around: you align only to 64 byte while running on a
> platform with 128 byte cache lines and non-coherent DMA.

Okay, I mistook your statement. This is indeed a valid statement.

>> And if platform is non-DMA-coherent then again it's the driver which
>> should take of coherency by using appropriate map/unmap APIs and
>> should avoid any cacheline sharing btw DMA buffer and skb_shared_info.
>
> The problem is that the streaming DMA API can only work correctly on
> cacheline-aligned buffers (because of the cache invalidation it performs
> for DMA ops; even with clean&invalidate, the operation isn't always safe
> if a cacheline is shared between DMA and CPU buffers). In the skb case,
> we could have the data potentially sharing the last addresses of a DMA
> buffer with struct skb_shared_info.
>
> We may be able to get away with SKB_DATA_ALIGN not using
> ARCH_DMA_MINALIGN *if* skb_shared_info is *not* written before or during
> an inbound DMA transfer (though such tricks are arch specific).
>
>> > IIUC, the Cavium platform has coherent DMA, so it shouldn't be an issue
>> > if we go back to 64 byte cache lines.
>>
>> Yes, Cavium platform is DMA coherent and there is no issue with reverting back
>> to 64byte cachelines. But do we want to do this because some platform has a
>> performance issue and this is an easy way to solve it. IMHO there seems
>> to be many ways to solve performance degradation within the driver itself, and
>> if those doesn't work then probably it makes sense to revert this.
>
> My initial thought was to revert the change because it was causing a
> significant performance regression on certain SoC. But given that it
> took over a year for people to follow up, it doesn't seem too urgent, so
> we should rather try to understand the issue and potential side effects
> of moving back to a 64 byte cache line.

Yes.

Thanks,
Sunil.

>
> --
> Catalin

[toc] | [prev] | [next] | [standalone]


#1625514

From"Chalamarla, Tirumalesh" <Tirumalesh.Chalamarla@cavium.com>
Date2017-04-18 20:30 +0200
Message-ID<txG30-10r-7@gated-at.bofh.it>
In reply to#1624583

On 4/17/17, 12:35 AM, "Imran Khan" <kimran@codeaurora.org> wrote:

    On 4/12/2017 7:30 PM, Chalamarla, Tirumalesh wrote:
    > 
    > 
    > On 4/11/17, 10:13 PM, "linux-arm-kernel on behalf of Imran Khan" <linux-arm-kernel-bounces@lists.infradead.org on behalf of kimran@codeaurora.org> wrote:
    > 
    >     On 4/7/2017 7:36 AM, Ganesh Mahendran wrote:
    >     > 2017-04-06 23:58 GMT+08:00 Catalin Marinas <catalin.marinas@arm.com>:
    >     >> On Thu, Apr 06, 2017 at 12:52:13PM +0530, Imran Khan wrote:
    >     >>> On 4/5/2017 10:13 AM, Imran Khan wrote:
    >     >>>>> We may have to revisit this logic and consider L1_CACHE_BYTES the
    >     >>>>> _minimum_ of cache line sizes in arm64 systems supported by the kernel.
    >     >>>>> Do you have any benchmarks on Cavium boards that would show significant
    >     >>>>> degradation with 64-byte L1_CACHE_BYTES vs 128?
    >     >>>>>
    >     >>>>> For non-coherent DMA, the simplest is to make ARCH_DMA_MINALIGN the
    >     >>>>> _maximum_ of the supported systems:
    >     >>>>>
    >     >>>>> diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
    >     >>>>> index 5082b30bc2c0..4b5d7b27edaf 100644
    >     >>>>> --- a/arch/arm64/include/asm/cache.h
    >     >>>>> +++ b/arch/arm64/include/asm/cache.h
    >     >>>>> @@ -18,17 +18,17 @@
    >     >>>>>
    >     >>>>>  #include <asm/cachetype.h>
    >     >>>>>
    >     >>>>> -#define L1_CACHE_SHIFT         7
    >     >>>>> +#define L1_CACHE_SHIFT         6
    >     >>>>>  #define L1_CACHE_BYTES         (1 << L1_CACHE_SHIFT)
    >     >>>>>
    >     >>>>>  /*
    >     >>>>>   * Memory returned by kmalloc() may be used for DMA, so we must make
    >     >>>>> - * sure that all such allocations are cache aligned. Otherwise,
    >     >>>>> - * unrelated code may cause parts of the buffer to be read into the
    >     >>>>> - * cache before the transfer is done, causing old data to be seen by
    >     >>>>> - * the CPU.
    >     >>>>> + * sure that all such allocations are aligned to the maximum *known*
    >     >>>>> + * cache line size on ARMv8 systems. Otherwise, unrelated code may cause
    >     >>>>> + * parts of the buffer to be read into the cache before the transfer is
    >     >>>>> + * done, causing old data to be seen by the CPU.
    >     >>>>>   */
    >     >>>>> -#define ARCH_DMA_MINALIGN      L1_CACHE_BYTES
    >     >>>>> +#define ARCH_DMA_MINALIGN      (128)
    >     >>>>>
    >     >>>>>  #ifndef __ASSEMBLY__
    >     >>>>>
    >     >>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
    >     >>>>> index 392c67eb9fa6..30bafca1aebf 100644
    >     >>>>> --- a/arch/arm64/kernel/cpufeature.c
    >     >>>>> +++ b/arch/arm64/kernel/cpufeature.c
    >     >>>>> @@ -976,9 +976,9 @@ void __init setup_cpu_features(void)
    >     >>>>>         if (!cwg)
    >     >>>>>                 pr_warn("No Cache Writeback Granule information, assuming
    >     >>>>> cache line size %d\n",
    >     >>>>>                         cls);
    >     >>>>> -       if (L1_CACHE_BYTES < cls)
    >     >>>>> -               pr_warn("L1_CACHE_BYTES smaller than the Cache Writeback Granule (%d < %d)\n",
    >     >>>>> -                       L1_CACHE_BYTES, cls);
    >     >>>>> +       if (ARCH_DMA_MINALIGN < cls)
    >     >>>>> +               pr_warn("ARCH_DMA_MINALIGN smaller than the Cache Writeback Granule (%d < %d)\n",
    >     >>>>> +                       ARCH_DMA_MINALIGN, cls);
    >     >>>>>  }
    >     >>>>>
    >     >>>>>  static bool __maybe_unused
    >     >>>>
    >     >>>> This change was discussed at: [1] but was not concluded as apparently no one
    >     >>>> came back with test report and numbers. After including this change in our
    >     >>>> local kernel we are seeing significant throughput improvement. For example with:
    >     >>>>
    >     >>>> iperf -c 192.168.1.181 -i 1 -w 128K -t 60
    >     >>>>
    >     >>>> The average throughput is improving by about 30% (230Mbps from 180Mbps).
    >     >>>> Could you please let us know if this change can be included in upstream kernel.
    >     >>>>
    >     >>>> [1]: https://groups.google.com/forum/#!topic/linux.kernel/P40yDB90ePs
    >     >>>
    >     >>> Could you please provide some feedback about the above mentioned query ?
    >     >>
    >     >> Do you have an explanation on the performance variation when
    >     >> L1_CACHE_BYTES is changed? We'd need to understand how the network stack
    >     >> is affected by L1_CACHE_BYTES, in which context it uses it (is it for
    >     >> non-coherent DMA?).
    >     > 
    >     > network stack use SKB_DATA_ALIGN to align.
    >     > ---
    >     > #define SKB_DATA_ALIGN(X) (((X) + (SMP_CACHE_BYTES - 1)) & \
    >     > ~(SMP_CACHE_BYTES - 1))
    >     > 
    >     > #define SMP_CACHE_BYTES L1_CACHE_BYTES
    >     > ---
    >     > I think this is the reason of performance regression.
    >     > 
    >     
    >     Yes this is the reason for performance regression. Due to increases L1 cache alignment the 
    >     object is coming from next kmalloc slab and skb->truesize is changing from 2304 bytes to 
    >     4352 bytes. This in turn increases sk_wmem_alloc which causes queuing of less send buffers.
    >     
    > We tried different benchmarks and found none which really affects with Cache line change. If there is no correctness issue,
    > I think we are fine with reverting the patch.
    > 
    So, can we revert the patch that makes L1_CACHE_SHIFT 7 or should the patch suggested by Catalin should be mainlined.
    We have verified the throughput degradation on 3.18 and 4.4 but I am afraid that this issue will be seen on other
    kernels too.
    > Though I still think it is beneficiary to do some more investigation for the perf loss, who knows 32 bit align or no align might 
    > Give even more perf benefit. 
    > 
    Which perf loss you are referring to here. Did you mean throughput loss here or some other perf benchmarking ?
    
The iperf issue mentioning here, looks to me as incomplete. 

    Thanks,
    Imran
    
    > 
    > Thanks,
    > Tirumalesh.  
    >     >>
    >     >> The Cavium guys haven't shown any numbers (IIUC) to back the
    >     >> L1_CACHE_BYTES performance improvement but I would not revert the
    >     >> original commit since ARCH_DMA_MINALIGN definitely needs to cover the
    >     >> maximum available cache line size, which is 128 for them.
    >     > 
    >     > how about define L1_CACHE_SHIFT like below:
    >     > ---
    >     > #ifdef CONFIG_ARM64_L1_CACHE_SHIFT
    >     > #define L1_CACHE_SHIFT CONFIG_ARM64_L1_CACHE_SHIFT
    >     > #else
    >     > #define L1_CACHE_SHIFT 7
    >     > endif
    >     > ---
    >     > 
    >     > Thanks
    >     > 
    >     >>
    >     >> --
    >     >> Catalin
    >     
    >     
    >     -- 
    >     QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
    >     
    >     _______________________________________________
    >     linux-arm-kernel mailing list
    >     linux-arm-kernel@lists.infradead.org
    >     http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
    >     
    > 
    
    
    -- 
    QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
    

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web