lkml.org 
[lkml]   [2022]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: ping //Re: [PATCH v2 0/2] squashfs: Add the mount parameter "threads="
From
Date
On 2022/8/29 7:18, Phillip Lougher wrote:
> On 26/08/2022 07:19, Xiaoming Ni wrote:
>> ping
>>
>>
>> On 2022/8/16 9:00, Xiaoming Ni wrote:
>>> Currently, Squashfs supports multiple decompressor parallel modes.
>>> However, this
>>> mode can be configured only during kernel building and does not
>>> support flexible
>>> selection during runtime.
>>>
>>> In the current patch set, the mount parameter "threads=" is added to
>>> allow users
>>> to select the parallel decompressor mode and configure the number of
>>> decompressors
>>> when mounting a file system.
>>>
>>> v2: fix warning: sparse: incorrect type in initializer (different
>>> address spaces)
>>>    Reported-by: kernel test robot <lkp@intel.com>
>
> I have made an initial review of the patches, and I have the following
> comments.
>
> Good things about the patch-series.
>
> 1. In principle I have no objections to making this configurable at
>    mount time.  But, a use-case for why this has become necessary
>    would help in the evaluation.
>
> 2. In general the code changes are good.  They are predominantly
>    exposing the existing different decompressor functionality into
>    structures which can be selected at mount time.  They do not
>    change existing functionality, and so there are no issues
>    about unexpected regressions.
>
> Things which I don't like about the patch-series.
>
> 1. There is no default kernel configuration option to keep the existing
>    behaviour, that is build time selectable only.  There may be many
>    companies/people where for "security" reasons the ability to
>    switch to a more CPU/memory intensive decompressor or more threads
>    is a risk.
>
>    Yes, I know the new kernel configuration options allow only the
>    selected default decompressor mode to be built.  In theory that
>    will restrict the available decompressors to the single decompressor
>    selected at build time.  So not much different to the current
>    position?  But, if the CONFIG_SQUASHFS_DECOMP_MULTI decompressor
>    is selected, that will now allow more threads to be used than is
No more threads than before the patch.

>    current, where it is currently restricted to num_online_cpus() * 2.
After the patch is installed, the maximum number of threads is still
num_online_cpus() * 2.

[PATCH v2 2/2] squashfs: Allows users to configure the number of
decompression threads

+#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
+ opts->thread_ops = &squashfs_decompressor_multi;
+ if (num > opts->thread_ops->max_decompressors())
+ num = opts->thread_ops->max_decompressors();
+ opts->thread_num = (int)num;
+ return 0;
+#else

>
> 2. You have decided to allow the mutiple decompressor implementations
>    to be selected at mount time - but you have also allowed only one
>    decompressor to be built at kernel build time.  This means you
>    end up in the fairly silly situation of having a mount time
>    option which allows the user to select between one decompressor.
>    There doesn't seem much point in having an option which allows
>    nothing to be changed.
When multiple decompression modes are selected during kernel build, or
only SQUASHFS_DECOMP_MULTI is selected during kernel build, the mount
parameter "threads=" is meaningful,
However, when only SQUASHFS_DECOMP_SINGLE or
SQUASHFS_DECOMP_MULTI_PERCPU is selected, the mount parameter "threads="
is meaningless.

Thank you for your guidance


>
> 3. Using thread=<number>, where thread=1 you use SQUASHFS_DECOMP_SINGLE
>    if it has been built, otherwise you fall back to
>    SQUASHFS_DECOMP_MULTI.  This meants the effect of thread=1 is
>    indeterminate and depends on the build options.  I would suggest
>    thread=1 should always mean use SQUASHFS_DECOMP_SINGLE.

SQUASHFS_DECOMP_MULTI and SQUASHFS_DECOMP_SINGLE are selected during
construction. Thread=1 indicates that SQUASHFS_DECOMP_SINGLEI is used.

If only SQUASHFS_DECOMP_MULTI is selected during construction, thread=1
indicates that SQUASHFS_DECOMP_MULTI is used, and only one decompression
thread is created.

Would it be better to provide more flexible mount options for images
that build only SQUASHFS_DECOMP_MULTI?

>
> 4. If SQUASHFS_DECOMP_MULTI is selected, there isn't a limit on the
>    maximum amount of threads allowed, and there is no ability to
>    set the maximum number of threads allowed at kernel build time
>    either.
After the patch is installed, the maximum number of threads is still
num_online_cpus() * 2.

[PATCH v2 2/2] squashfs: Allows users to configure the number of
decompression threads

+#ifdef CONFIG_SQUASHFS_DECOMP_MULTI
+ opts->thread_ops = &squashfs_decompressor_multi;
+ if (num > opts->thread_ops->max_decompressors())
+ num = opts->thread_ops->max_decompressors();
+ opts->thread_num = (int)num;
+ return 0;

Did I misunderstand your question?


>
> All of the above seems to be a bit of a mess.
>
> As regards points 1 - 3, personally I would add a default kernel
> configuration option that keeps the existing behaviour, build time
> selectable only, no additional mount time options.  Then a
> kernel configuration option that allows the different decompressors
> to be selected at mount time, but which always builds all the
> decompressors.  This will avoid the silliness of point 2, and
Would it be better to allow flexible selection of decompression mode
combinations?

> the indeterminate behaviour of point 3.
>
> As regards point 4, I think you should require the maximum number
> of threads allowable to be determined at build time, this is
> good for security and avoids attempts to use too much CPU
> and memory.  The default at kernel build time should be minimal,
> to avoid cases where an unchanged value can cause a potential
> security hazard on a low end system.  In otherwords it is
> up to the user at build time to set the value to an appropriate
> value for their system.
In patch 2, the maximum number of threads has been limited,
Have I misunderstood your question


>
> Phillip
>
> ---
> Phillip Lougher, Squashfs author and maintainer.
>

Thanks
Xiaoming Ni

>>>
>>> v1:
>>> https://lore.kernel.org/lkml/20220815031100.75243-1-nixiaoming@huawei.com/
>>>
>>> ----
>>>
>>> Xiaoming Ni (2):
>>>    squashfs: add the mount parameter theads=<single|multi|percpu>
>>>    squashfs: Allows users to configure the number of decompression
>>>      threads.
>>>
>>>   fs/squashfs/Kconfig                     | 24 ++++++++--
>>>   fs/squashfs/decompressor_multi.c        | 32 ++++++++------
>>>   fs/squashfs/decompressor_multi_percpu.c | 39 ++++++++++-------
>>>   fs/squashfs/decompressor_single.c       | 23 ++++++----
>>>   fs/squashfs/squashfs.h                  | 39 ++++++++++++++---
>>>   fs/squashfs/squashfs_fs_sb.h            |  4 +-
>>>   fs/squashfs/super.c                     | 77
>>> ++++++++++++++++++++++++++++++++-
>>>   7 files changed, 192 insertions(+), 46 deletions(-)
>>>
>>
>
>
> .

\
 
 \ /
  Last update: 2022-08-30 15:42    [W:0.719 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site