Re: [PATCH v7 4/4] fs: unicode: Add utf8 module and a unicode layer
On 09/04/21 12:40 am, Gabriel Krisman Bertazi wrote: Shreeya Patel writes: utf8data.h_shipped has a large database table which is an auto-generated decodification trie for the unicode normalization functions. It is not necessary to load this large table in the kernel if no filesystem is using it, hence make UTF-8 encoding loadable by converting it into a module. Modify the file called unicode-core which will act as a layer for unicode subsystem. It will load the UTF-8 module and access it's functions whenever any filesystem that needs unicode is mounted. Currently, only UTF-8 encoding is supported but if any other encodings are supported in future then the layer file would be responsible for loading the desired encoding module. Also, indirect calls using function pointers are slow, use static calls to avoid overhead caused in case of repeated indirect calls. Static calls improves the performance by directly calling the functions as opposed to indirect calls. Signed-off-by: Shreeya Patel --- Changes in v7 - Update the help text in Kconfig - Handle the unicode_load_static_call function failure by decrementing the reference. - Correct the code for handling built-in utf8 option as well. - Correct the synchronization for accessing utf8mod. - Make changes to unicode_unload() for handling the situation where utf8mod != NULL and um == NULL. Changes in v6 - Add spinlock to protect utf8mod and avoid NULL pointer dereference. - Change the static call function names for being consistent with kernel coding style. - Merge the unicode_load_module function with unicode_load as it is not really needed to have a separate function. - Use try_then_module_get instead of module_get to avoid loading the module even when it is already loaded. - Improve the commit message. Changes in v5 - Rename global variables and default static call functions for better understanding - Make only config UNICODE_UTF8 visible and config UNICODE to be always enabled provided UNICODE_UTF8 is enabled. - Improve the documentation for Kconfig - Improve the commit message. Changes in v4 - Return error from the static calls instead of doing nothing and succeeding even without loading the module. - Remove the complete usage of utf8_ops and use static calls at all places. - Restore the static calls to default values when module is unloaded. - Decrement the reference of module after calling the unload function. - Remove spinlock as there will be no race conditions after removing utf8_ops. Changes in v3 - Add a patch which checks if utf8 is loaded before calling utf8_unload() in ext4 and f2fs filesystems - Return error if strscpy() returns value < 0 - Correct the conditions to prevent NULL pointer dereference while accessing functions via utf8_ops variable. - Add spinlock to avoid race conditions. - Use static_call() for preventing speculative execution attacks. Changes in v2 - Remove the duplicate file from the last patch. - Make the wrapper functions inline. - Remove msleep and use try_module_get() and module_put() for ensuring that module is loaded correctly and also doesn't get unloaded while in use. - Resolve the warning reported by kernel test robot. - Resolve all the checkpatch.pl warnings. fs/unicode/Kconfig| 26 +++- fs/unicode/Makefile | 5 +- fs/unicode/unicode-core.c | 297 ++ fs/unicode/unicode-utf8.c | 264 + include/linux/unicode.h | 96 ++-- 5 files changed, 483 insertions(+), 205 deletions(-) create mode 100644 fs/unicode/unicode-utf8.c diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig index 2c27b9a5cd6c..0c69800a2a37 100644 --- a/fs/unicode/Kconfig +++ b/fs/unicode/Kconfig @@ -2,13 +2,31 @@ # # UTF-8 normalization # +# CONFIG_UNICODE will be automatically enabled if CONFIG_UNICODE_UTF8 +# is enabled. This config option adds the unicode subsystem layer which loads +# the UTF-8 module whenever any filesystem needs it. config UNICODE - bool "UTF-8 normalization and casefolding support" + bool + +config UNICODE_UTF8 + tristate "UTF-8 module" "UTF-8 module" is the text that will appear in menuconfig and other configuration utilities. This string not very helpful to describe what this code is about or why it is different from NLS_utf8. People come to this option looking for the case-insensitive feature in ext4, so I'd prefer to keep the mention to 'casefolding'. or even improve the original a bit to say: tristate: "UTF-8 support for native Case-Insensitive filesystems" Other than these and what Eric mentioned, the code looks good to me. Thanks Gabriel and Eric for your reviews. I gave this series a try and it seems to work fine. It does raise a new warning, though /home/krisman/src/linux/fs/unicode/unicode-core.c: In
Re: [PATCH v7 4/4] fs: unicode: Add utf8 module and a unicode layer
Shreeya Patel writes: > utf8data.h_shipped has a large database table which is an auto-generated > decodification trie for the unicode normalization functions. > It is not necessary to load this large table in the kernel if no > filesystem is using it, hence make UTF-8 encoding loadable by converting > it into a module. > > Modify the file called unicode-core which will act as a layer for > unicode subsystem. It will load the UTF-8 module and access it's functions > whenever any filesystem that needs unicode is mounted. > Currently, only UTF-8 encoding is supported but if any other encodings > are supported in future then the layer file would be responsible for > loading the desired encoding module. > > Also, indirect calls using function pointers are slow, use static calls to > avoid overhead caused in case of repeated indirect calls. Static calls > improves the performance by directly calling the functions as opposed to > indirect calls. > > Signed-off-by: Shreeya Patel > --- > Changes in v7 > - Update the help text in Kconfig > - Handle the unicode_load_static_call function failure by decrementing > the reference. > - Correct the code for handling built-in utf8 option as well. > - Correct the synchronization for accessing utf8mod. > - Make changes to unicode_unload() for handling the situation where > utf8mod != NULL and um == NULL. > > Changes in v6 > - Add spinlock to protect utf8mod and avoid NULL pointer > dereference. > - Change the static call function names for being consistent with > kernel coding style. > - Merge the unicode_load_module function with unicode_load as it is > not really needed to have a separate function. > - Use try_then_module_get instead of module_get to avoid loading the > module even when it is already loaded. > - Improve the commit message. > > Changes in v5 > - Rename global variables and default static call functions for better > understanding > - Make only config UNICODE_UTF8 visible and config UNICODE to be always > enabled provided UNICODE_UTF8 is enabled. > - Improve the documentation for Kconfig > - Improve the commit message. > > Changes in v4 > - Return error from the static calls instead of doing nothing and > succeeding even without loading the module. > - Remove the complete usage of utf8_ops and use static calls at all > places. > - Restore the static calls to default values when module is unloaded. > - Decrement the reference of module after calling the unload function. > - Remove spinlock as there will be no race conditions after removing > utf8_ops. > > Changes in v3 > - Add a patch which checks if utf8 is loaded before calling utf8_unload() > in ext4 and f2fs filesystems > - Return error if strscpy() returns value < 0 > - Correct the conditions to prevent NULL pointer dereference while > accessing functions via utf8_ops variable. > - Add spinlock to avoid race conditions. > - Use static_call() for preventing speculative execution attacks. > > Changes in v2 > - Remove the duplicate file from the last patch. > - Make the wrapper functions inline. > - Remove msleep and use try_module_get() and module_put() > for ensuring that module is loaded correctly and also > doesn't get unloaded while in use. > - Resolve the warning reported by kernel test robot. > - Resolve all the checkpatch.pl warnings. > > fs/unicode/Kconfig| 26 +++- > fs/unicode/Makefile | 5 +- > fs/unicode/unicode-core.c | 297 ++ > fs/unicode/unicode-utf8.c | 264 + > include/linux/unicode.h | 96 ++-- > 5 files changed, 483 insertions(+), 205 deletions(-) > create mode 100644 fs/unicode/unicode-utf8.c > > diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig > index 2c27b9a5cd6c..0c69800a2a37 100644 > --- a/fs/unicode/Kconfig > +++ b/fs/unicode/Kconfig > @@ -2,13 +2,31 @@ > # > # UTF-8 normalization > # > +# CONFIG_UNICODE will be automatically enabled if CONFIG_UNICODE_UTF8 > +# is enabled. This config option adds the unicode subsystem layer which loads > +# the UTF-8 module whenever any filesystem needs it. > config UNICODE > - bool "UTF-8 normalization and casefolding support" > + bool > + > +config UNICODE_UTF8 > + tristate "UTF-8 module" "UTF-8 module" is the text that will appear in menuconfig and other configuration utilities. This string not very helpful to describe what this code is about or why it is different from NLS_utf8. People come to this option looking for the case-insensitive feature in ext4, so I'd prefer to keep the mention to 'casefolding'. or even improve the original a bit to say: tristate: "UTF-8 support for native Case-Insensitive filesystems" Other than these and what Eric mentioned, the code looks good to me. I gave this series a try and it seems to work fine. It does raise a new warning, though
Re: [PATCH v7 4/4] fs: unicode: Add utf8 module and a unicode layer
On Wed, Apr 07, 2021 at 08:18:45PM +0530, Shreeya Patel wrote: > diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig > index 2c27b9a5cd6c..0c69800a2a37 100644 > --- a/fs/unicode/Kconfig > +++ b/fs/unicode/Kconfig > @@ -2,13 +2,31 @@ > # > # UTF-8 normalization > # > +# CONFIG_UNICODE will be automatically enabled if CONFIG_UNICODE_UTF8 > +# is enabled. This config option adds the unicode subsystem layer which loads > +# the UTF-8 module whenever any filesystem needs it. > config UNICODE > - bool "UTF-8 normalization and casefolding support" > + bool > + > +config UNICODE_UTF8 > + tristate "UTF-8 module" > + select UNICODE > help > - Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding > - support. > + Say M here to enable UTF-8 NFD normalization and NFD+CF casefolding > + support as a loadable module or say Y for building it into the kernel. > + > + utf8data.h_shipped has a large database table which is an > + auto-generated decodification trie for the unicode normalization > + functions and it is not necessary to carry this large table in the > + kernel. Hence, enabling UNICODE_UTF8 as M will allow you to avoid > + carrying this large table into the kernel and module will only be > + loaded whenever required by any filesystem. > + Please note, in this case utf8 module will only be available after > + booting into the compiled kernel. If your filesystem requires it to > + have utf8 during boot time then you should have it built into the > + kernel by saying Y here to avoid boot failure. This help text seems to contradict itself; it says "it is not necessary to carry this large table in the kernel", and then later it says that in some cases it is in fact necessary. It would also be helpful for the help text to mention which filesystems actually support this feature. > diff --git a/fs/unicode/unicode-core.c b/fs/unicode/unicode-core.c > index 730dbaedf593..d9e9e410893d 100644 > --- a/fs/unicode/unicode-core.c > +++ b/fs/unicode/unicode-core.c > @@ -1,228 +1,132 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > #include > #include > -#include > #include > -#include > #include > #include > -#include > +#include > > -#include "utf8n.h" > +DEFINE_SPINLOCK(utf8mod_lock); This spinlock should be 'static'. - Eric
