* Re: [bp:tip-x86-alternatives 1/1] error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type
From: Miguel Ojeda @ 2023-01-07 0:38 UTC (permalink / raw)
To: Borislav Petkov
Cc: kernel test robot, llvm, oe-kbuild-all, rust-for-linux, lkml,
Yujie Liu
In-Reply-To: <Y7i1h3lCMKfxB532@zn.tnic>
On Sat, Jan 7, 2023 at 12:58 AM Borislav Petkov <bp@alien8.de> wrote:
>
> I figured as much.
Sorry, I didn't mean otherwise, I should have quoted the next paragraph.
You are of course right that the instructions are not complete, I just
meant to add a bit of context, i.e. that Rust got enabled due to the
config, but as far as I understand, it shouldn't be getting enabled in
the other ones for the moment.
> No need - I ran it by hand just to show that I don't have a rust compiler
> installed.
My point was that the script expects some variables set by `Makefile`,
similar to `$CC` etc., so that output does not imply you have (or not)
a suitable Rust toolchain installed (i.e. it will currently also fail
if you have it installed).
> Bottom line is: if I get a build report involving a rust compiler, there better
> be in the reproduction instructions a hint how to install one so that I can
> reproduce. Alternatively, I can always simply ignore it.
Cc'ing Yujie for the robot instructions. Once I reported something
missing in the instructions (unrelated to Rust) and they were happy to
get the report, so I assume they will want to improve it here too.
Meanwhile (of course it is not the same as proper reproduction
instructions since the LKP team may do something different), the
documentation on how to set it up for a normal developer is at:
https://www.kernel.org/doc/html/latest/rust/quick-start.html, in case
it helps (if you are up for it... :)
> And while we're reporting bugs: the error message from the compiler itself could
> use some "humanization" - I have zero clue what it is trying to tell me.
What would you want to see? We can ask the relevant Rust team to see
if they can improve it.
In general, note that you can ask `rustc` to further explain an error
giving it the code with `--explain`. The compiler suggests this
itself, but sadly the robot cut it out :(
For more information about this error, try `rustc --explain E0588`
In this case, it gives:
A type with `packed` representation hint has a field with `align`
representation hint.
Erroneous code example:
```
#[repr(align(16))]
struct Aligned(i32);
#[repr(packed)] // error!
struct Packed(Aligned);
```
Just like you cannot have both `align` and `packed` representation
hints on a
same type, a `packed` type cannot contain another type with the `align`
representation hint. However, you can do the opposite:
```
#[repr(packed)]
struct Packed(i32);
#[repr(align(16))] // ok!
struct Aligned(Packed);
```
You can also see it rendered in
https://doc.rust-lang.org/error_codes/E0588.html, which is also useful
if you don't have the toolchain around. Another option if you don't
remember the URL is going to Compiler Explorer, e.g.
https://godbolt.org/z/Ec17xnGsT.
Yujie: perhaps the robot could avoid dropping that line? Or even
better, you could automatically add a URL like above and/or run the
compiler with `--explain` and add it directly to the output (with a
size limit I guess)? That could probably be very helpful for kernel
developers that receive a Rust report.
Cheers,
Miguel
^ permalink raw reply
* Re: [bp:tip-x86-alternatives 1/1] error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type
From: Borislav Petkov @ 2023-01-06 23:57 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: kernel test robot, llvm, oe-kbuild-all, rust-for-linux, lkml
In-Reply-To: <CANiq72kc60aPcx5LwFhOGL4AXOhZsZj32iOg75K5ZxLaaRaYkg@mail.gmail.com>
On Sat, Jan 07, 2023 at 12:25:17AM +0100, Miguel Ojeda wrote:
> Yeah, note the x86_64-rhel-8.3-rust config name. It is a config the
> kernel test robot added for testing with Rust enabled (which explains
> the version text you saw for `rustc`).
I figured as much.
As I said:
"These reproduction instructions look insufficient to me."
> The script is meant to be called as a Make target, e.g. `make LLVM=1
> rustavailable`. Perhaps we can put a message if the script was called
> directly.
No need - I ran it by hand just to show that I don't have a rust compiler
installed.
Bottom line is: if I get a build report involving a rust compiler, there better
be in the reproduction instructions a hint how to install one so that I can
reproduce. Alternatively, I can always simply ignore it.
And while we're reporting bugs: the error message from the compiler itself could
use some "humanization" - I have zero clue what it is trying to tell me.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [bp:tip-x86-alternatives 1/1] error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type
From: Miguel Ojeda @ 2023-01-06 23:26 UTC (permalink / raw)
To: Alexander Altman
Cc: Borislav Petkov, kernel test robot, llvm, oe-kbuild-all,
rust-for-linux, lkml
In-Reply-To: <76353487-736A-4470-AD31-77F47F8C08F6@me.com>
On Tue, Dec 27, 2022 at 9:31 PM Alexander Altman <alexanderaltman@me.com> wrote:
>
> One way to resolve this temporarily would be to add the following line above
> the offending struct:
> /// <div rustbindgen hide></div>
> This will cause bindgen to ignore the struct entirely and not translate it. If it’s
> actually needed for Rust code, now or later, then we can’t do that and need
> to actually replace it with something translatable, or else leave it hidden and
> manually create its translation on the Rust side. For the latter, just using a
> u32 for the entire bitfield-containing union would be sufficient.
Thanks a lot Alexander for taking a look!
This is https://github.com/rust-lang/rust-bindgen/issues/2179.
What we do for constructs that `bindgen` cannot map is to add the
appropriate parameter/line in `rust/bindgen_parameters`. We hit a
similar case for `x86_msi_data` that you can see in that file. So
please feel free to add it there.
If we end up needing to access it from the Rust side, another
alternative is to write a C function that performs the required
operation on it that then the Rust side calls.
Cheers,
Miguel
^ permalink raw reply
* Re: [bp:tip-x86-alternatives 1/1] error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type
From: Miguel Ojeda @ 2023-01-06 23:25 UTC (permalink / raw)
To: Borislav Petkov
Cc: kernel test robot, llvm, oe-kbuild-all, rust-for-linux, lkml
In-Reply-To: <Y6r4mXz5NS0+HVXo@zn.tnic>
Hi Borislav,
On Tue, Dec 27, 2022 at 2:52 PM Borislav Petkov <bp@alien8.de> wrote:
>
> These reproduction instructions look insufficient to me. The env needs a
> rust compiler installed. Which I don't have:
Yeah, note the x86_64-rhel-8.3-rust config name. It is a config the
kernel test robot added for testing with Rust enabled (which explains
the version text you saw for `rustc`).
> ./scripts/rust_is_available.sh -v
> ***
> *** Rust compiler '' could not be found.
> ***
The script is meant to be called as a Make target, e.g. `make LLVM=1
rustavailable`. Perhaps we can put a message if the script was called
directly.
Cheers,
Miguel
^ permalink raw reply
* Re: Rust in-kernel TLS handshake
From: Vincenzo Palazzo @ 2023-01-06 9:57 UTC (permalink / raw)
To: FUJITA Tomonori, rust-for-linux
In-Reply-To: <CAJ+Zx=3OwXeJNsz8DLp_94ciWpQKoiV75v=XH3FoUQKW==_S8w@mail.gmail.com>
This sounds pretty exciting, and I think to play with it a little
the bit will help to see what the future looks like, also this
seems a really good use case of rust in the Kernel.
Let me know if there is a need for help, I had some free time in
the next coming months.
Cheers!
Vincent.
On Wed Dec 28, 2022 at 2:33 AM CET, FUJITA Tomonori wrote:
> Hi,
>
> I've started in-kernel TLS handshake implementation in Rust.
> https://github.com/fujita/rust-tls
>
> There is some debate over in-kernel TLS handshake mainly because of
> the complexity. I'd like to see if Rust could help with auditing such
> complicated security-relevant code in the kernel.
>
> I worked on QUIC as a consumer of TLS. This can establish a QUIC connection
> with Quinn's example client, Rust QUIC implementation. Only minimum
> server side functionality and connection establishment are supported.
>
> From the perspective of Rust-for-Linux, the main work is implementing
> APIs for crypto subsystem. Also libraries such as working with buffers
> (like Tokio's bytes) would be helpful, I think (should be useful for
> other projects). I'll work for mainline. Meanwhile you can compile
> this kernel module
> with my fork.
>
> Opinions?
>
> Resend due to a delivery issue. Sorry if you got this twice.
^ permalink raw reply
* Re: [PATCH 5/6] fixdep: avoid parsing the same file over again
From: Vincenzo @ 2023-01-06 9:31 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: linux-kernel, Miguel Ojeda, Alex Gaynor, Björn Roy Baron,
Boqun Feng, Gary Guo, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Wedson Almeida Filho, rust-for-linux
In-Reply-To: <20221231064203.1623793-6-masahiroy@kernel.org>
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
On Sat Dec 31, 2022 at 7:42 AM CET, Masahiro Yamada wrote:
> The dep files (*.d files) emitted by C compilers usually contain the
> deduplicated list of included files.
>
> There is an exceptional case; if a header is included by the -include
> command line option, and also by #include directive, it appears twice
> in the *.d file.
>
> For example, the top Makefile specifies the command line option,
> -include $(srctree)/include/linux/kconfig.h. You do not need to
> add #include <linux/kconfig.h> in every source file.
>
> In fact, include/linux/kconfig.h is listed twice in many .*.cmd files
> due to include/linux/xarray.h including <linux/kconfig.h>.
> I did not fix that since it is a small redundancy.
>
> However, this is more annoying for rustc. rustc emits the dependency
> for each emission type.
>
> For example, cmd_rustc_library emits dep-info, obj, and metadata.
> So, the emitted *.d file contains the dependency for those 3 targets,
> which makes fixdep parse the same file 3 times.
>
> $ grep rust/alloc/raw_vec.rs rust/.alloc.o.cmd
> rust/alloc/raw_vec.rs \
> rust/alloc/raw_vec.rs \
> rust/alloc/raw_vec.rs \
>
> To skip the second parsing, this commit adds a hash table for parsed
> files, just like we did for CONFIG options.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> scripts/basic/fixdep.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index b20777b888d7..cc8f6d34c2ca 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -113,7 +113,7 @@ struct item {
> };
>
> #define HASHSZ 256
> -static struct item *config_hashtab[HASHSZ];
> +static struct item *config_hashtab[HASHSZ], *file_hashtab[HASHSZ];
>
> static unsigned int strhash(const char *str, unsigned int sz)
> {
> @@ -361,6 +361,10 @@ static void parse_dep_file(char *p, const char *target)
> * name, which will be the original one, and ignore any
> * other source names, which will be intermediate
> * temporary files.
> + *
> + * rustc emits the same dependency list for each
> + * emission type. It is enough to list the source name
> + * just once.
> */
> if (!saw_any_target) {
> saw_any_target = true;
> @@ -368,7 +372,8 @@ static void parse_dep_file(char *p, const char *target)
> printf("deps_%s := \\\n", target);
> need_parse = true;
> }
> - } else if (!is_ignored_file(p, q - p)) {
> + } else if (!is_ignored_file(p, q - p) &&
> + !in_hashtable(p, q - p, file_hashtab)) {
> printf(" %s \\\n", p);
> need_parse = true;
> }
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH 3/6] kbuild: remove sed commands after rustc rules
From: Vincenzo @ 2023-01-06 9:28 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: linux-kernel, Miguel Ojeda, Alex Gaynor, Björn Roy Baron,
Boqun Feng, Gary Guo, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Tom Rix, Wedson Almeida Filho, llvm,
rust-for-linux
In-Reply-To: <20221231064203.1623793-4-masahiroy@kernel.org>
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
> rustc may put comments in dep-info, so sed is used to drop them before
> passing it to fixdep.
>
> Now that fixdep can remove comments, Makefiles do not need to run sed.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> rust/Makefile | 6 ++----
> scripts/Makefile.build | 18 ++++--------------
> scripts/Makefile.host | 3 +--
> 3 files changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/rust/Makefile b/rust/Makefile
> index 0e2a32f4b3e9..c8941fec6955 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -333,8 +333,7 @@ quiet_cmd_rustc_procmacro = $(RUSTC_OR_CLIPPY_QUIET) P $@
> $(RUSTC_OR_CLIPPY) $(rust_common_flags) \
> --emit=dep-info=$(depfile) --emit=link=$@ --extern proc_macro \
> --crate-type proc-macro \
> - --crate-name $(patsubst lib%.so,%,$(notdir $@)) $<; \
> - sed -i '/^\#/d' $(depfile)
> + --crate-name $(patsubst lib%.so,%,$(notdir $@)) $<
>
> # Procedural macros can only be used with the `rustc` that compiled it.
> # Therefore, to get `libmacros.so` automatically recompiled when the compiler
> @@ -349,8 +348,7 @@ quiet_cmd_rustc_library = $(if $(skip_clippy),RUSTC,$(RUSTC_OR_CLIPPY_QUIET)) L
> $(filter-out $(skip_flags),$(rust_flags) $(rustc_target_flags)) \
> --emit=dep-info=$(depfile) --emit=obj=$@ --emit=metadata=$(dir $@)$(patsubst %.o,lib%.rmeta,$(notdir $@)) \
> --crate-type rlib -L$(objtree)/$(obj) \
> - --crate-name $(patsubst %.o,%,$(notdir $@)) $<; \
> - sed -i '/^\#/d' $(depfile) \
> + --crate-name $(patsubst %.o,%,$(notdir $@)) $< \
> $(if $(rustc_objcopy),;$(OBJCOPY) $(rustc_objcopy) $@)
>
> rust-analyzer:
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 40de20246e50..76323201232a 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -289,9 +289,6 @@ rust_common_cmd = \
> --crate-name $(basename $(notdir $@)) \
> --emit=dep-info=$(depfile)
>
> -rust_handle_depfile = \
> - sed -i '/^\#/d' $(depfile)
> -
> # `--emit=obj`, `--emit=asm` and `--emit=llvm-ir` imply a single codegen unit
> # will be used. We explicitly request `-Ccodegen-units=1` in any case, and
> # the compiler shows a warning if it is not 1. However, if we ever stop
> @@ -301,9 +298,7 @@ rust_handle_depfile = \
> # would not match each other.
>
> quiet_cmd_rustc_o_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@
> - cmd_rustc_o_rs = \
> - $(rust_common_cmd) --emit=obj=$@ $<; \
> - $(rust_handle_depfile)
> + cmd_rustc_o_rs = $(rust_common_cmd) --emit=obj=$@ $<
>
> $(obj)/%.o: $(src)/%.rs FORCE
> $(call if_changed_dep,rustc_o_rs)
> @@ -311,24 +306,19 @@ $(obj)/%.o: $(src)/%.rs FORCE
> quiet_cmd_rustc_rsi_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@
> cmd_rustc_rsi_rs = \
> $(rust_common_cmd) -Zunpretty=expanded $< >$@; \
> - command -v $(RUSTFMT) >/dev/null && $(RUSTFMT) $@; \
> - $(rust_handle_depfile)
> + command -v $(RUSTFMT) >/dev/null && $(RUSTFMT) $@
>
> $(obj)/%.rsi: $(src)/%.rs FORCE
> $(call if_changed_dep,rustc_rsi_rs)
>
> quiet_cmd_rustc_s_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@
> - cmd_rustc_s_rs = \
> - $(rust_common_cmd) --emit=asm=$@ $<; \
> - $(rust_handle_depfile)
> + cmd_rustc_s_rs = $(rust_common_cmd) --emit=asm=$@ $<
>
> $(obj)/%.s: $(src)/%.rs FORCE
> $(call if_changed_dep,rustc_s_rs)
>
> quiet_cmd_rustc_ll_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@
> - cmd_rustc_ll_rs = \
> - $(rust_common_cmd) --emit=llvm-ir=$@ $<; \
> - $(rust_handle_depfile)
> + cmd_rustc_ll_rs = $(rust_common_cmd) --emit=llvm-ir=$@ $<
>
> $(obj)/%.ll: $(src)/%.rs FORCE
> $(call if_changed_dep,rustc_ll_rs)
> diff --git a/scripts/Makefile.host b/scripts/Makefile.host
> index 4434cdbf7b8e..bc782655d09e 100644
> --- a/scripts/Makefile.host
> +++ b/scripts/Makefile.host
> @@ -148,8 +148,7 @@ $(host-cxxobjs): $(obj)/%.o: $(src)/%.cc FORCE
> # host-rust -> Executable
> quiet_cmd_host-rust = HOSTRUSTC $@
> cmd_host-rust = \
> - $(HOSTRUSTC) $(hostrust_flags) --emit=link=$@ $<; \
> - sed -i '/^\#/d' $(depfile)
> + $(HOSTRUSTC) $(hostrust_flags) --emit=link=$@ $<
> $(host-rust): $(obj)/%: $(src)/%.rs FORCE
> $(call if_changed_dep,host-rust)
>
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH 1/6] kbuild: specify output names separately for each emission type from rustc
From: Vincenzo @ 2023-01-06 9:24 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: linux-kernel, Miguel Ojeda, Alex Gaynor, Björn Roy Baron,
Boqun Feng, Gary Guo, Nathan Chancellor, Nick Desaulniers,
Nicolas Schier, Tom Rix, Wedson Almeida Filho, llvm,
rust-for-linux
In-Reply-To: <20221231064203.1623793-2-masahiroy@kernel.org>
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
On Sat Dec 31, 2022 at 7:41 AM CET, Masahiro Yamada wrote:
> In Kbuild, two different rules must not write to the same file, but
> it happens when compiling rust source files.
>
> For example, set CONFIG_SAMPLE_RUST_MINIMAL=m and run the following:
>
> $ make -j$(nproc) samples/rust/rust_minimal.o samples/rust/rust_minimal.rsi \
> samples/rust/rust_minimal.s samples/rust/rust_minimal.ll
> [snip]
> RUSTC [M] samples/rust/rust_minimal.o
> RUSTC [M] samples/rust/rust_minimal.rsi
> RUSTC [M] samples/rust/rust_minimal.s
> RUSTC [M] samples/rust/rust_minimal.ll
> mv: cannot stat 'samples/rust/rust_minimal.d': No such file or directory
> make[3]: *** [scripts/Makefile.build:334: samples/rust/rust_minimal.ll] Error 1
> make[3]: *** Waiting for unfinished jobs....
> mv: cannot stat 'samples/rust/rust_minimal.d': No such file or directory
> make[3]: *** [scripts/Makefile.build:309: samples/rust/rust_minimal.o] Error 1
> mv: cannot stat 'samples/rust/rust_minimal.d': No such file or directory
> make[3]: *** [scripts/Makefile.build:326: samples/rust/rust_minimal.s] Error 1
> make[2]: *** [scripts/Makefile.build:504: samples/rust] Error 2
> make[1]: *** [scripts/Makefile.build:504: samples] Error 2
> make: *** [Makefile:2008: .] Error 2
>
> The reason for the error is that 4 threads running in parallel creates
> and renames the same file path, samples/rust/rust_minimal.d.
>
> This does not happen when compiling C or assembly files because we
> explicitly specify the dependency filename by using the preprocessor
> option, -Wp,-MMD,$(depfile). $(depfile) is a unique path for each target.
>
> Currently, rustc is only given --out-dir and the list of emitted types.
> So, all the rust build rules output the dep-info into the default
> <CRATE_NAME>.d, causing the conflict.
>
> Fortunately, the --emit option is able to specify the output path
> individually, with the form --emit=<type>=<path>.
>
> Add --emit=dep-info=$(depfile) to the common command part. Also, remove
> the redundant --out-dir because we specify the output path for each type.
>
> The code gets much cleaner because we do not need to rename *.d files.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> rust/Makefile | 10 ++++------
> scripts/Makefile.build | 14 +++++++-------
> scripts/Makefile.host | 9 +++------
> 3 files changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/rust/Makefile b/rust/Makefile
> index ff70c4c916f8..0e2a32f4b3e9 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -331,10 +331,9 @@ $(obj)/exports_kernel_generated.h: $(obj)/kernel.o FORCE
> quiet_cmd_rustc_procmacro = $(RUSTC_OR_CLIPPY_QUIET) P $@
> cmd_rustc_procmacro = \
> $(RUSTC_OR_CLIPPY) $(rust_common_flags) \
> - --emit=dep-info,link --extern proc_macro \
> - --crate-type proc-macro --out-dir $(objtree)/$(obj) \
> + --emit=dep-info=$(depfile) --emit=link=$@ --extern proc_macro \
> + --crate-type proc-macro \
> --crate-name $(patsubst lib%.so,%,$(notdir $@)) $<; \
> - mv $(objtree)/$(obj)/$(patsubst lib%.so,%,$(notdir $@)).d $(depfile); \
> sed -i '/^\#/d' $(depfile)
>
> # Procedural macros can only be used with the `rustc` that compiled it.
> @@ -348,10 +347,9 @@ quiet_cmd_rustc_library = $(if $(skip_clippy),RUSTC,$(RUSTC_OR_CLIPPY_QUIET)) L
> OBJTREE=$(abspath $(objtree)) \
> $(if $(skip_clippy),$(RUSTC),$(RUSTC_OR_CLIPPY)) \
> $(filter-out $(skip_flags),$(rust_flags) $(rustc_target_flags)) \
> - --emit=dep-info,obj,metadata --crate-type rlib \
> - --out-dir $(objtree)/$(obj) -L$(objtree)/$(obj) \
> + --emit=dep-info=$(depfile) --emit=obj=$@ --emit=metadata=$(dir $@)$(patsubst %.o,lib%.rmeta,$(notdir $@)) \
> + --crate-type rlib -L$(objtree)/$(obj) \
> --crate-name $(patsubst %.o,%,$(notdir $@)) $<; \
> - mv $(objtree)/$(obj)/$(patsubst %.o,%,$(notdir $@)).d $(depfile); \
> sed -i '/^\#/d' $(depfile) \
> $(if $(rustc_objcopy),;$(OBJCOPY) $(rustc_objcopy) $@)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index a0d5c6cca76d..40de20246e50 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -285,11 +285,11 @@ rust_common_cmd = \
> -Zcrate-attr=no_std \
> -Zcrate-attr='feature($(rust_allowed_features))' \
> --extern alloc --extern kernel \
> - --crate-type rlib --out-dir $(obj) -L $(objtree)/rust/ \
> - --crate-name $(basename $(notdir $@))
> + --crate-type rlib -L $(objtree)/rust/ \
> + --crate-name $(basename $(notdir $@)) \
> + --emit=dep-info=$(depfile)
>
> rust_handle_depfile = \
> - mv $(obj)/$(basename $(notdir $@)).d $(depfile); \
> sed -i '/^\#/d' $(depfile)
>
> # `--emit=obj`, `--emit=asm` and `--emit=llvm-ir` imply a single codegen unit
> @@ -302,7 +302,7 @@ rust_handle_depfile = \
>
> quiet_cmd_rustc_o_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@
> cmd_rustc_o_rs = \
> - $(rust_common_cmd) --emit=dep-info,obj $<; \
> + $(rust_common_cmd) --emit=obj=$@ $<; \
> $(rust_handle_depfile)
>
> $(obj)/%.o: $(src)/%.rs FORCE
> @@ -310,7 +310,7 @@ $(obj)/%.o: $(src)/%.rs FORCE
>
> quiet_cmd_rustc_rsi_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@
> cmd_rustc_rsi_rs = \
> - $(rust_common_cmd) --emit=dep-info -Zunpretty=expanded $< >$@; \
> + $(rust_common_cmd) -Zunpretty=expanded $< >$@; \
> command -v $(RUSTFMT) >/dev/null && $(RUSTFMT) $@; \
> $(rust_handle_depfile)
>
> @@ -319,7 +319,7 @@ $(obj)/%.rsi: $(src)/%.rs FORCE
>
> quiet_cmd_rustc_s_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@
> cmd_rustc_s_rs = \
> - $(rust_common_cmd) --emit=dep-info,asm $<; \
> + $(rust_common_cmd) --emit=asm=$@ $<; \
> $(rust_handle_depfile)
>
> $(obj)/%.s: $(src)/%.rs FORCE
> @@ -327,7 +327,7 @@ $(obj)/%.s: $(src)/%.rs FORCE
>
> quiet_cmd_rustc_ll_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@
> cmd_rustc_ll_rs = \
> - $(rust_common_cmd) --emit=dep-info,llvm-ir $<; \
> + $(rust_common_cmd) --emit=llvm-ir=$@ $<; \
> $(rust_handle_depfile)
>
> $(obj)/%.ll: $(src)/%.rs FORCE
> diff --git a/scripts/Makefile.host b/scripts/Makefile.host
> index da133780b751..4434cdbf7b8e 100644
> --- a/scripts/Makefile.host
> +++ b/scripts/Makefile.host
> @@ -84,8 +84,8 @@ _hostc_flags = $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS) \
> $(HOSTCFLAGS_$(target-stem).o)
> _hostcxx_flags = $(KBUILD_HOSTCXXFLAGS) $(HOST_EXTRACXXFLAGS) \
> $(HOSTCXXFLAGS_$(target-stem).o)
> -_hostrust_flags = $(KBUILD_HOSTRUSTFLAGS) $(HOST_EXTRARUSTFLAGS) \
> - $(HOSTRUSTFLAGS_$(target-stem))
> +hostrust_flags = $(KBUILD_HOSTRUSTFLAGS) $(HOST_EXTRARUSTFLAGS) \
> + $(HOSTRUSTFLAGS_$(target-stem)) --emit=dep-info=$(depfile)
>
> # $(objtree)/$(obj) for including generated headers from checkin source files
> ifeq ($(KBUILD_EXTMOD),)
> @@ -97,7 +97,6 @@ endif
>
> hostc_flags = -Wp,-MMD,$(depfile) $(_hostc_flags)
> hostcxx_flags = -Wp,-MMD,$(depfile) $(_hostcxx_flags)
> -hostrust_flags = $(_hostrust_flags)
>
> #####
> # Compile programs on the host
> @@ -149,9 +148,7 @@ $(host-cxxobjs): $(obj)/%.o: $(src)/%.cc FORCE
> # host-rust -> Executable
> quiet_cmd_host-rust = HOSTRUSTC $@
> cmd_host-rust = \
> - $(HOSTRUSTC) $(hostrust_flags) --emit=dep-info,link \
> - --out-dir=$(obj)/ $<; \
> - mv $(obj)/$(target-stem).d $(depfile); \
> + $(HOSTRUSTC) $(hostrust_flags) --emit=link=$@ $<; \
> sed -i '/^\#/d' $(depfile)
> $(host-rust): $(obj)/%: $(src)/%.rs FORCE
> $(call if_changed_dep,host-rust)
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH] scripts: avoid redundant re-reading makefiles.
From: Miguel Ojeda @ 2023-01-05 23:01 UTC (permalink / raw)
To: Alexander Pantyukhin; +Cc: ojeda, rust-for-linux, akpm, Asahi Lina
In-Reply-To: <CAPi66w-2aV8CQs6xGSR4xZn4+39HdUqvwTZEidYKqUR-11thDA@mail.gmail.com>
On Thu, Jan 5, 2023 at 8:51 PM Alexander Pantyukhin
<apantykhin@gmail.com> wrote:
>
> I checked files now. It seems only one place in /samples has 2 files
> with 1 makefile in the parent folder. So it looks like the patch is
> untimely for now.
Yeah, though you could do a benchmark with more files to account for
the future :)
> I see, thanks. Still have a question about using "with". Is it reasonable?
It is definitely reasonable and you cannot go wrong with `with`. I am
not sure if matters much in this case (short-lived script, read-only,
not a huge amount of files...), and with CPython the objects are
destroyed immediately anyway I believe.
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH] scripts: avoid redundant re-reading makefiles.
From: Alexander Pantyukhin @ 2023-01-05 19:51 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: ojeda, rust-for-linux, akpm, Asahi Lina
In-Reply-To: <CANiq72=mgZCC7ECEO=bqLQ2rJKQjqS7Sg7CW51_mpX4VcG7o6A@mail.gmail.com>
Hi Miguel.
Please see my comments below.
чт, 5 янв. 2023 г. в 22:44, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>:
>
> Hi Alex,
>
> The commit message is looking better, thank you. Please see comments
> inline below.
>
> On Wed, Jan 4, 2023 at 5:04 PM Alexander Pantyukhin
> <apantykhin@gmail.com> wrote:
> >
> > The main goal of the patch is avoid redundant re-reading makefiles
> > if these were read before. This is performance improvement.
>
> Like it was mentioned in v1, could you please give a rough estimate
> about what is the improvement from old to new? (in particular, without
> the other changes). That way, others can get an idea whether the code
> complexity is worth it or not.
I checked files now. It seems only one place in /samples has 2 files
with 1 makefile in the parent folder. So it looks like the patch is
untimely for now.
>
> > Additional code improvements are using 'with' when read file and check
> > if file exists.
>
> Note that others submitted an improvement for the missing file case as
> we discussed in v1, so it would be better if they submitted their own
> patches for those. I am Cc'ing them so that they have a chance to be
> aware of this patch and potentially submit theirs.
>
> In any case, their approach seems more idiomatic for Python [1], so I
> think we should do it that way.
>
> Finally, the title of this commit should probably start with the
> "scripts: generate_rust_analyzer: " prefix, otherwise it is a bit too
> broad.
I see, thanks. Still have a question about using "with". Is it reasonable?
>
> Thanks!
>
> Cheers,
> Miguel
>
> [1] https://docs.python.org/3.11/glossary.html#term-EAFP
Thank you!
Best, Alex.
^ permalink raw reply
* Re: [PATCH] scripts: store Makefiles in dictionary
From: Miguel Ojeda @ 2023-01-05 18:48 UTC (permalink / raw)
To: Alexander Pantyukhin; +Cc: ojeda, rust-for-linux, akpm
In-Reply-To: <CAPi66w_zdf73em8vb7WxCP33ST+R_VzCz-qWvckuZuzLagVQPA@mail.gmail.com>
Hi Alex,
On Thu, Jan 5, 2023 at 9:02 AM Alexander Pantyukhin
<apantykhin@gmail.com> wrote:
>
> I changed the Subject name, so now it became a new letter in the mailing list.
> Now it's a ` [PATCH] scripts: avoid redundant re-reading makefiles `
Yeah, no worries -- please mark new revisions with a "[PATCH v2]" etc.
as needed when you want to signify that (`git-format-patch` can do
this with the `-v` flag), and ideally add a lore.kernel.org link to
the previous version in the new one's changelog or cover letter if you
think it may be unclear where it is coming from.
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH] scripts: avoid redundant re-reading makefiles.
From: Miguel Ojeda @ 2023-01-05 18:44 UTC (permalink / raw)
To: Alexander Pantyukhin; +Cc: ojeda, rust-for-linux, akpm, Asahi Lina
In-Reply-To: <20230104160405.5530-1-apantykhin@gmail.com>
Hi Alex,
The commit message is looking better, thank you. Please see comments
inline below.
On Wed, Jan 4, 2023 at 5:04 PM Alexander Pantyukhin
<apantykhin@gmail.com> wrote:
>
> The main goal of the patch is avoid redundant re-reading makefiles
> if these were read before. This is performance improvement.
Like it was mentioned in v1, could you please give a rough estimate
about what is the improvement from old to new? (in particular, without
the other changes). That way, others can get an idea whether the code
complexity is worth it or not.
> Additional code improvements are using 'with' when read file and check
> if file exists.
Note that others submitted an improvement for the missing file case as
we discussed in v1, so it would be better if they submitted their own
patches for those. I am Cc'ing them so that they have a chance to be
aware of this patch and potentially submit theirs.
In any case, their approach seems more idiomatic for Python [1], so I
think we should do it that way.
Finally, the title of this commit should probably start with the
"scripts: generate_rust_analyzer: " prefix, otherwise it is a bit too
broad.
Thanks!
Cheers,
Miguel
[1] https://docs.python.org/3.11/glossary.html#term-EAFP
^ permalink raw reply
* Re: [PATCH 4/7] rust: sync: introduce `ArcBorrow`
From: Wedson Almeida Filho @ 2023-01-04 17:52 UTC (permalink / raw)
To: Emilio Cobos Álvarez
Cc: Gary Guo, rust-for-linux, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Björn Roy Baron, linux-kernel
In-Reply-To: <df2f6c0b-fb4c-97e5-b607-7df626ea5933@crisal.io>
On Wed, 4 Jan 2023 at 16:06, Emilio Cobos Álvarez <emilio@crisal.io> wrote:
>
> Sorry for the drive-by comment, but maybe it saves some work.
>
> On 1/4/23 16:29, Wedson Almeida Filho wrote:
> > On Sat, 31 Dec 2022 at 19:43, Gary Guo <gary@garyguo.net> wrote:
> >>
> >> On Wed, 28 Dec 2022 06:03:43 +0000
> >> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> >>
> >>> This allows us to create references to a ref-counted allocation without
> >>> double-indirection and that still allow us to increment the refcount to
> >>> a new `Arc<T>`.
> >>>
> >>> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> >>> ---
> >>> rust/kernel/sync.rs | 2 +-
> >>> rust/kernel/sync/arc.rs | 97 +++++++++++++++++++++++++++++++++++++++++
> >>> 2 files changed, 98 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> >>> index 39b379dd548f..5de03ea83ea1 100644
> >>> --- a/rust/kernel/sync.rs
> >>> +++ b/rust/kernel/sync.rs
> >>> @@ -7,4 +7,4 @@
> >>>
> >>> mod arc;
> >>>
> >>> -pub use arc::Arc;
> >>> +pub use arc::{Arc, ArcBorrow};
> >>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> >>> index dbc7596cc3ce..f68bfc02c81a 100644
> >>> --- a/rust/kernel/sync/arc.rs
> >>> +++ b/rust/kernel/sync/arc.rs
> >>> @@ -19,6 +19,7 @@ use crate::{bindings, error::Result, types::Opaque};
> >>> use alloc::boxed::Box;
> >>> use core::{
> >>> marker::{PhantomData, Unsize},
> >>> + mem::ManuallyDrop,
> >>> ops::Deref,
> >>> ptr::NonNull,
> >>> };
> >>> @@ -164,6 +165,18 @@ impl<T: ?Sized> Arc<T> {
> >>> _p: PhantomData,
> >>> }
> >>> }
> >>> +
> >>> + /// Returns an [`ArcBorrow`] from the given [`Arc`].
> >>> + ///
> >>> + /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
> >>> + /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised.
> >>> + #[inline]
> >>> + pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> {
> >>> + // SAFETY: The constraint that the lifetime of the shared reference must outlive that of
> >>> + // the returned `ArcBorrow` ensures that the object remains alive and that no mutable
> >>> + // reference can be created.
> >>> + unsafe { ArcBorrow::new(self.ptr) }
> >>> + }
> >>> }
> >>>
> >>> impl<T: ?Sized> Deref for Arc<T> {
> >>> @@ -208,3 +221,87 @@ impl<T: ?Sized> Drop for Arc<T> {
> >>> }
> >>> }
> >>> }
> >>> +
> >>> +/// A borrowed reference to an [`Arc`] instance.
> >>> +///
> >>> +/// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler
> >>> +/// to use just `&T`, which we can trivially get from an `Arc<T>` instance.
> >>> +///
> >>> +/// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
> >>> +/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
> >>> +/// to a pointer (`Arc<T>`) to the object (`T`). An [`ArcBorrow`] eliminates this double
> >>> +/// indirection while still allowing one to increment the refcount and getting an `Arc<T>` when/if
> >>> +/// needed.
> >>> +///
> >>> +/// # Invariants
> >>> +///
> >>> +/// There are no mutable references to the underlying [`Arc`], and it remains valid for the
> >>> +/// lifetime of the [`ArcBorrow`] instance.
> >>> +///
> >>> +/// # Example
> >>> +///
> >>> +/// ```
> >>> +/// use crate::sync::{Arc, ArcBorrow};
> >>> +///
> >>> +/// struct Example;
> >>> +///
> >>> +/// fn do_something(e: ArcBorrow<'_, Example>) -> Arc<Example> {
> >>> +/// e.into()
> >>> +/// }
> >>> +///
> >>> +/// let obj = Arc::try_new(Example)?;
> >>> +/// let cloned = do_something(obj.as_arc_borrow());
> >>> +///
> >>> +/// // Assert that both `obj` and `cloned` point to the same underlying object.
> >>> +/// assert!(core::ptr::eq(&*obj, &*cloned));
> >>> +/// ```
> >>> +pub struct ArcBorrow<'a, T: ?Sized + 'a> {
> >>> + inner: NonNull<ArcInner<T>>,
> >>> + _p: PhantomData<&'a ()>,
> >>> +}
> >>> +
> >>> +impl<T: ?Sized> Clone for ArcBorrow<'_, T> {
> >>> + fn clone(&self) -> Self {
> >>> + *self
> >>> + }
> >>> +}
> >>> +
> >>> +impl<T: ?Sized> Copy for ArcBorrow<'_, T> {}
> >>
> >> Couldn't this just be derived `Clone` and `Copy`?
> >
> > Indeed. I'll send a v2 with this.
>
> I'm not sure this is true. Deriving will add the T: Copy and T: Clone
> bound, which I think is not what you want here.
>
> i.e., I assume you want an ArcBorrow to be Copy even if the underlying T
> is not.
>
> See <https://github.com/rust-lang/rust/issues/26925> for the relevant
> (really long-standing) Rust issue.
Thanks for the heads up, Emilio!
After trying this out, derive doesn't work. The errors brought me back
memories of when I first implemented this over a year ago, though I
didn't take the time to try to understand why it was failing.
So no v2. The series will remain as is.
Cheers
>
> Cheers,
>
> -- Emilio
^ permalink raw reply
* Re: [PATCH 7/7] rust: sync: add support for dispatching on Arc and ArcBorrow.
From: Vincenzo @ 2023-01-04 16:26 UTC (permalink / raw)
To: Wedson Almeida Filho, rust-for-linux
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, linux-kernel
In-Reply-To: <20221228060346.352362-7-wedsonaf@gmail.com>
On Wed Dec 28, 2022 at 7:03 AM CET, Wedson Almeida Filho wrote:
> Trait objects (`dyn T`) require trait `T` to be "object safe". One of
> the requirements for "object safety" is that the receiver have one of
> the allowed types. This commit adds `Arc<T>` and `ArcBorrow<'_, T>` to
> the list of allowed types.
>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/sync/arc.rs | 20 ++++++++++++++++++--
> 2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 4bde65e7b06b..e0b0e953907d 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -15,6 +15,7 @@
> #![feature(allocator_api)]
> #![feature(coerce_unsized)]
> #![feature(core_ffi_c)]
> +#![feature(dispatch_from_dyn)]
> #![feature(receiver_trait)]
> #![feature(unsize)]
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 832bafc74a90..ff73f9240ca1 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -92,9 +92,15 @@ use core::{
> /// Coercion from `Arc<Example>` to `Arc<dyn MyTrait>`:
> ///
> /// ```
> -/// use kernel::sync::Arc;
> +/// use kernel::sync::{Arc, ArcBorrow};
> +///
> +/// trait MyTrait {
> +/// // Trait has a function whose `self` type is `Arc<Self>`.
> +/// fn example1(self: Arc<Self>) {}
> ///
> -/// trait MyTrait {}
> +/// // Trait has a function whose `self` type is `ArcBorrow<'_, Self>`.
> +/// fn example2(self: ArcBorrow<'_, Self>) {}
> +/// }
> ///
> /// struct Example;
> /// impl MyTrait for Example {}
> @@ -123,6 +129,9 @@ impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
> // dynamically-sized type (DST) `U`.
> impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::CoerceUnsized<Arc<U>> for Arc<T> {}
>
> +// This is to allow `Arc<U>` to be dispatched on when `Arc<T>` can be coerced into `Arc<U>`.
> +impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<Arc<U>> for Arc<T> {}
> +
> // SAFETY: It is safe to send `Arc<T>` to another thread when the underlying `T` is `Sync` because
> // it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> // `T` to be `Send` because any thread that has an `Arc<T>` may ultimately access `T` directly, for
> @@ -297,6 +306,13 @@ pub struct ArcBorrow<'a, T: ?Sized + 'a> {
> // This is to allow [`ArcBorrow`] (and variants) to be used as the type of `self`.
> impl<T: ?Sized> core::ops::Receiver for ArcBorrow<'_, T> {}
>
> +// This is to allow `ArcBorrow<U>` to be dispatched on when `ArcBorrow<T>` can be coerced into
> +// `ArcBorrow<U>`.
> +impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<ArcBorrow<'_, U>>
> + for ArcBorrow<'_, T>
> +{
> +}
> +
> impl<T: ?Sized> Clone for ArcBorrow<'_, T> {
> fn clone(&self) -> Self {
> *self
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH 6/7] rust: sync: introduce `UniqueArc`
From: Vincenzo @ 2023-01-04 16:21 UTC (permalink / raw)
To: Wedson Almeida Filho, rust-for-linux
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, linux-kernel
In-Reply-To: <20221228060346.352362-6-wedsonaf@gmail.com>
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
On Wed Dec 28, 2022 at 7:03 AM CET, Wedson Almeida Filho wrote:
> Since `Arc<T>` does not allow mutating `T` directly (i.e., without inner
> mutability), it is currently not possible to do some initialisation of
> `T` post construction but before being shared.
>
> `UniqueArc<T>` addresses this problem essentially being an `Arc<T>` that
> has a refcount of 1 and is therefore writable. Once initialisation is
> completed, it can be transitioned (without failure paths) into an
> `Arc<T>`.
>
> Suggested-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> ---
> rust/kernel/sync.rs | 2 +-
> rust/kernel/sync/arc.rs | 152 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 151 insertions(+), 3 deletions(-)
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 5de03ea83ea1..33da23e3076d 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -7,4 +7,4 @@
>
> mod arc;
>
> -pub use arc::{Arc, ArcBorrow};
> +pub use arc::{Arc, ArcBorrow, UniqueArc};
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 84f31c85a513..832bafc74a90 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -19,8 +19,9 @@ use crate::{bindings, error::Result, types::Opaque};
> use alloc::boxed::Box;
> use core::{
> marker::{PhantomData, Unsize},
> - mem::ManuallyDrop,
> - ops::Deref,
> + mem::{ManuallyDrop, MaybeUninit},
> + ops::{Deref, DerefMut},
> + pin::Pin,
> ptr::NonNull,
> };
>
> @@ -222,6 +223,19 @@ impl<T: ?Sized> Drop for Arc<T> {
> }
> }
>
> +impl<T: ?Sized> From<UniqueArc<T>> for Arc<T> {
> + fn from(item: UniqueArc<T>) -> Self {
> + item.inner
> + }
> +}
> +
> +impl<T: ?Sized> From<Pin<UniqueArc<T>>> for Arc<T> {
> + fn from(item: Pin<UniqueArc<T>>) -> Self {
> + // SAFETY: The type invariants of `Arc` guarantee that the data is pinned.
> + unsafe { Pin::into_inner_unchecked(item).inner }
> + }
> +}
> +
> /// A borrowed reference to an [`Arc`] instance.
> ///
> /// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler
> @@ -328,3 +342,137 @@ impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
> unsafe { &self.inner.as_ref().data }
> }
> }
> +
> +/// A refcounted object that is known to have a refcount of 1.
> +///
> +/// It is mutable and can be converted to an [`Arc`] so that it can be shared.
> +///
> +/// # Invariants
> +///
> +/// `inner` always has a reference count of 1.
> +///
> +/// # Examples
> +///
> +/// In the following example, we make changes to the inner object before turning it into an
> +/// `Arc<Test>` object (after which point, it cannot be mutated directly). Note that `x.into()`
> +/// cannot fail.
> +///
> +/// ```
> +/// use kernel::sync::{Arc, UniqueArc};
> +///
> +/// struct Example {
> +/// a: u32,
> +/// b: u32,
> +/// }
> +///
> +/// fn test() -> Result<Arc<Example>> {
> +/// let mut x = UniqueArc::try_new(Example { a: 10, b: 20 })?;
> +/// x.a += 1;
> +/// x.b += 1;
> +/// Ok(x.into())
> +/// }
> +///
> +/// # test().unwrap();
> +/// ```
> +///
> +/// In the following example we first allocate memory for a ref-counted `Example` but we don't
> +/// initialise it on allocation. We do initialise it later with a call to [`UniqueArc::write`],
> +/// followed by a conversion to `Arc<Example>`. This is particularly useful when allocation happens
> +/// in one context (e.g., sleepable) and initialisation in another (e.g., atomic):
> +///
> +/// ```
> +/// use kernel::sync::{Arc, UniqueArc};
> +///
> +/// struct Example {
> +/// a: u32,
> +/// b: u32,
> +/// }
> +///
> +/// fn test() -> Result<Arc<Example>> {
> +/// let x = UniqueArc::try_new_uninit()?;
> +/// Ok(x.write(Example { a: 10, b: 20 }).into())
> +/// }
> +///
> +/// # test().unwrap();
> +/// ```
> +///
> +/// In the last example below, the caller gets a pinned instance of `Example` while converting to
> +/// `Arc<Example>`; this is useful in scenarios where one needs a pinned reference during
> +/// initialisation, for example, when initialising fields that are wrapped in locks.
> +///
> +/// ```
> +/// use kernel::sync::{Arc, UniqueArc};
> +///
> +/// struct Example {
> +/// a: u32,
> +/// b: u32,
> +/// }
> +///
> +/// fn test() -> Result<Arc<Example>> {
> +/// let mut pinned = Pin::from(UniqueArc::try_new(Example { a: 10, b: 20 })?);
> +/// // We can modify `pinned` because it is `Unpin`.
> +/// pinned.as_mut().a += 1;
> +/// Ok(pinned.into())
> +/// }
> +///
> +/// # test().unwrap();
> +/// ```
> +pub struct UniqueArc<T: ?Sized> {
> + inner: Arc<T>,
> +}
> +
> +impl<T> UniqueArc<T> {
> + /// Tries to allocate a new [`UniqueArc`] instance.
> + pub fn try_new(value: T) -> Result<Self> {
> + Ok(Self {
> + // INVARIANT: The newly-created object has a ref-count of 1.
> + inner: Arc::try_new(value)?,
> + })
> + }
> +
> + /// Tries to allocate a new [`UniqueArc`] instance whose contents are not initialised yet.
> + pub fn try_new_uninit() -> Result<UniqueArc<MaybeUninit<T>>> {
> + Ok(UniqueArc::<MaybeUninit<T>> {
> + // INVARIANT: The newly-created object has a ref-count of 1.
> + inner: Arc::try_new(MaybeUninit::uninit())?,
> + })
> + }
> +}
> +
> +impl<T> UniqueArc<MaybeUninit<T>> {
> + /// Converts a `UniqueArc<MaybeUninit<T>>` into a `UniqueArc<T>` by writing a value into it.
> + pub fn write(mut self, value: T) -> UniqueArc<T> {
> + self.deref_mut().write(value);
> + let inner = ManuallyDrop::new(self).inner.ptr;
> + UniqueArc {
> + // SAFETY: The new `Arc` is taking over `ptr` from `self.inner` (which won't be
> + // dropped). The types are compatible because `MaybeUninit<T>` is compatible with `T`.
> + inner: unsafe { Arc::from_inner(inner.cast()) },
> + }
> + }
> +}
> +
> +impl<T: ?Sized> From<UniqueArc<T>> for Pin<UniqueArc<T>> {
> + fn from(obj: UniqueArc<T>) -> Self {
> + // SAFETY: It is not possible to move/replace `T` inside a `Pin<UniqueArc<T>>` (unless `T`
> + // is `Unpin`), so it is ok to convert it to `Pin<UniqueArc<T>>`.
> + unsafe { Pin::new_unchecked(obj) }
> + }
> +}
> +
> +impl<T: ?Sized> Deref for UniqueArc<T> {
> + type Target = T;
> +
> + fn deref(&self) -> &Self::Target {
> + self.inner.deref()
> + }
> +}
> +
> +impl<T: ?Sized> DerefMut for UniqueArc<T> {
> + fn deref_mut(&mut self) -> &mut Self::Target {
> + // SAFETY: By the `Arc` type invariant, there is necessarily a reference to the object, so
> + // it is safe to dereference it. Additionally, we know there is only one reference when
> + // it's inside a `UniqueArc`, so it is safe to get a mutable reference.
> + unsafe { &mut self.inner.ptr.as_mut().data }
> + }
> +}
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH 5/7] rust: sync: allow type of `self` to be `ArcBorrow<T>`
From: Vincenzo @ 2023-01-04 16:18 UTC (permalink / raw)
To: Wedson Almeida Filho, rust-for-linux
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, linux-kernel
In-Reply-To: <20221228060346.352362-5-wedsonaf@gmail.com>
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
On Wed Dec 28, 2022 at 7:03 AM CET, Wedson Almeida Filho wrote:
> This allows associated functions whose `self` argument has
> `ArcBorrow<T>` as their type. This, in turn, allows callers to use the
> dot syntax to make calls.
>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> ---
> rust/kernel/sync/arc.rs | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index f68bfc02c81a..84f31c85a513 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -255,11 +255,34 @@ impl<T: ?Sized> Drop for Arc<T> {
> /// // Assert that both `obj` and `cloned` point to the same underlying object.
> /// assert!(core::ptr::eq(&*obj, &*cloned));
> /// ```
> +///
> +/// Using `ArcBorrow<T>` as the type of `self`:
> +///
> +/// ```
> +/// use crate::sync::{Arc, ArcBorrow};
> +///
> +/// struct Example {
> +/// a: u32,
> +/// b: u32,
> +/// }
> +///
> +/// impl Example {
> +/// fn use_reference(self: ArcBorrow<'_, Self>) {
> +/// // ...
> +/// }
> +/// }
> +///
> +/// let obj = Arc::try_new(Example { a: 10, b: 20 })?;
> +/// obj.as_arc_borrow().use_reference();
> +/// ```
> pub struct ArcBorrow<'a, T: ?Sized + 'a> {
> inner: NonNull<ArcInner<T>>,
> _p: PhantomData<&'a ()>,
> }
>
> +// This is to allow [`ArcBorrow`] (and variants) to be used as the type of `self`.
> +impl<T: ?Sized> core::ops::Receiver for ArcBorrow<'_, T> {}
> +
> impl<T: ?Sized> Clone for ArcBorrow<'_, T> {
> fn clone(&self) -> Self {
> *self
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH 3/7] rust: sync: allow coercion from `Arc<T>` to `Arc<U>`
From: Vincenzo @ 2023-01-04 16:13 UTC (permalink / raw)
To: Wedson Almeida Filho, rust-for-linux
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, linux-kernel
In-Reply-To: <20221228060346.352362-3-wedsonaf@gmail.com>
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
On Wed Dec 28, 2022 at 7:03 AM CET, Wedson Almeida Filho wrote:
> The coercion is only allowed if `U` is a compatible dynamically-sized
> type (DST). For example, if we have some type `X` that implements trait
> `Y`, then this allows `Arc<X>` to be coerced into `Arc<dyn Y>`.
>
> Suggested-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> ---
> rust/kernel/lib.rs | 2 ++
> rust/kernel/sync/arc.rs | 27 ++++++++++++++++++++++++++-
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 1a10f7c0ddd9..4bde65e7b06b 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -13,8 +13,10 @@
>
> #![no_std]
> #![feature(allocator_api)]
> +#![feature(coerce_unsized)]
> #![feature(core_ffi_c)]
> #![feature(receiver_trait)]
> +#![feature(unsize)]
>
> // Ensure conditional compilation based on the kernel configuration works;
> // otherwise we may silently break things like initcall handling.
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index e2eb0e67d483..dbc7596cc3ce 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -17,7 +17,11 @@
>
> use crate::{bindings, error::Result, types::Opaque};
> use alloc::boxed::Box;
> -use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
> +use core::{
> + marker::{PhantomData, Unsize},
> + ops::Deref,
> + ptr::NonNull,
> +};
>
> /// A reference-counted pointer to an instance of `T`.
> ///
> @@ -82,6 +86,23 @@ use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
> /// obj.use_reference();
> /// obj.take_over();
> /// ```
> +///
> +/// Coercion from `Arc<Example>` to `Arc<dyn MyTrait>`:
> +///
> +/// ```
> +/// use kernel::sync::Arc;
> +///
> +/// trait MyTrait {}
> +///
> +/// struct Example;
> +/// impl MyTrait for Example {}
> +///
> +/// // `obj` has type `Arc<Example>`.
> +/// let obj: Arc<Example> = Arc::try_new(Example)?;
> +///
> +/// // `coerced` has type `Arc<dyn MyTrait>`.
> +/// let coerced: Arc<dyn MyTrait> = obj;
> +/// ```
> pub struct Arc<T: ?Sized> {
> ptr: NonNull<ArcInner<T>>,
> _p: PhantomData<ArcInner<T>>,
> @@ -96,6 +117,10 @@ struct ArcInner<T: ?Sized> {
> // This is to allow [`Arc`] (and variants) to be used as the type of `self`.
> impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
>
> +// This is to allow coercion from `Arc<T>` to `Arc<U>` if `T` can be converted to the
> +// dynamically-sized type (DST) `U`.
> +impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::CoerceUnsized<Arc<U>> for Arc<T> {}
> +
> // SAFETY: It is safe to send `Arc<T>` to another thread when the underlying `T` is `Sync` because
> // it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> // `T` to be `Send` because any thread that has an `Arc<T>` may ultimately access `T` directly, for
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH 2/7] rust: sync: allow type of `self` to be `Arc<T>` or variants
From: Vincenzo @ 2023-01-04 16:12 UTC (permalink / raw)
To: Wedson Almeida Filho, rust-for-linux
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, linux-kernel
In-Reply-To: <20221228060346.352362-2-wedsonaf@gmail.com>
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
On Wed Dec 28, 2022 at 7:03 AM CET, Wedson Almeida Filho wrote:
> This allows associated functions whose `self` argument has `Arc<T>` or
> variants as their type. This, in turn, allows callers to use the dot
> syntax to make calls.
>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/sync/arc.rs | 28 ++++++++++++++++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ace064a3702a..1a10f7c0ddd9 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -14,6 +14,7 @@
> #![no_std]
> #![feature(allocator_api)]
> #![feature(core_ffi_c)]
> +#![feature(receiver_trait)]
>
> // Ensure conditional compilation based on the kernel configuration works;
> // otherwise we may silently break things like initcall handling.
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 22290eb5ab9b..e2eb0e67d483 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -57,6 +57,31 @@ use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
> ///
> /// // The refcount drops to zero when `cloned` goes out of scope, and the memory is freed.
> /// ```
> +///
> +/// Using `Arc<T>` as the type of `self`:
> +///
> +/// ```
> +/// use kernel::sync::Arc;
> +///
> +/// struct Example {
> +/// a: u32,
> +/// b: u32,
> +/// }
> +///
> +/// impl Example {
> +/// fn take_over(self: Arc<Self>) {
> +/// // ...
> +/// }
> +///
> +/// fn use_reference(self: &Arc<Self>) {
> +/// // ...
> +/// }
> +/// }
> +///
> +/// let obj = Arc::try_new(Example { a: 10, b: 20 })?;
> +/// obj.use_reference();
> +/// obj.take_over();
> +/// ```
> pub struct Arc<T: ?Sized> {
> ptr: NonNull<ArcInner<T>>,
> _p: PhantomData<ArcInner<T>>,
> @@ -68,6 +93,9 @@ struct ArcInner<T: ?Sized> {
> data: T,
> }
>
> +// This is to allow [`Arc`] (and variants) to be used as the type of `self`.
> +impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
> +
> // SAFETY: It is safe to send `Arc<T>` to another thread when the underlying `T` is `Sync` because
> // it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> // `T` to be `Send` because any thread that has an `Arc<T>` may ultimately access `T` directly, for
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH 1/7] rust: sync: add `Arc` for ref-counted allocations
From: Vincenzo @ 2023-01-04 16:08 UTC (permalink / raw)
To: Wedson Almeida Filho, rust-for-linux
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, linux-kernel, Will Deacon, Peter Zijlstra,
Mark Rutland
In-Reply-To: <20221228060346.352362-1-wedsonaf@gmail.com>
On Wed Dec 28, 2022 at 7:03 AM CET, Wedson Almeida Filho wrote:
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
> This is a basic implementation of `Arc` backed by C's `refcount_t`. It
> allows Rust code to idiomatically allocate memory that is ref-counted.
>
> Cc: Will Deacon <will@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/bindings/lib.rs | 1 +
> rust/helpers.c | 19 ++++
> rust/kernel/lib.rs | 1 +
> rust/kernel/sync.rs | 10 ++
> rust/kernel/sync/arc.rs | 157 ++++++++++++++++++++++++++++++++
> 6 files changed, 189 insertions(+)
> create mode 100644 rust/kernel/sync.rs
> create mode 100644 rust/kernel/sync/arc.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c48bc284214a..75d85bd6c592 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/slab.h>
> +#include <linux/refcount.h>
>
> /* `bindgen` gets confused at certain things. */
> const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs
> index 6c50ee62c56b..7b246454e009 100644
> --- a/rust/bindings/lib.rs
> +++ b/rust/bindings/lib.rs
> @@ -41,6 +41,7 @@ mod bindings_raw {
> #[allow(dead_code)]
> mod bindings_helper {
> // Import the generated bindings for types.
> + use super::bindings_raw::*;
> include!(concat!(
> env!("OBJTREE"),
> "/rust/bindings/bindings_helpers_generated.rs"
> diff --git a/rust/helpers.c b/rust/helpers.c
> index b4f15eee2ffd..09a4d93f9d62 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -20,6 +20,7 @@
>
> #include <linux/bug.h>
> #include <linux/build_bug.h>
> +#include <linux/refcount.h>
>
> __noreturn void rust_helper_BUG(void)
> {
> @@ -27,6 +28,24 @@ __noreturn void rust_helper_BUG(void)
> }
> EXPORT_SYMBOL_GPL(rust_helper_BUG);
>
> +refcount_t rust_helper_REFCOUNT_INIT(int n)
> +{
> + return (refcount_t)REFCOUNT_INIT(n);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_REFCOUNT_INIT);
> +
> +void rust_helper_refcount_inc(refcount_t *r)
> +{
> + refcount_inc(r);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_refcount_inc);
> +
> +bool rust_helper_refcount_dec_and_test(refcount_t *r)
> +{
> + return refcount_dec_and_test(r);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
> +
> /*
> * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 53040fa9e897..ace064a3702a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -31,6 +31,7 @@ mod static_assert;
> #[doc(hidden)]
> pub mod std_vendor;
> pub mod str;
> +pub mod sync;
> pub mod types;
>
> #[doc(hidden)]
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> new file mode 100644
> index 000000000000..39b379dd548f
> --- /dev/null
> +++ b/rust/kernel/sync.rs
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Synchronisation primitives.
> +//!
> +//! This module contains the kernel APIs related to synchronisation that have been ported or
> +//! wrapped for usage by Rust code in the kernel.
> +
> +mod arc;
> +
> +pub use arc::Arc;
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> new file mode 100644
> index 000000000000..22290eb5ab9b
> --- /dev/null
> +++ b/rust/kernel/sync/arc.rs
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! A reference-counted pointer.
> +//!
> +//! This module implements a way for users to create reference-counted objects and pointers to
> +//! them. Such a pointer automatically increments and decrements the count, and drops the
> +//! underlying object when it reaches zero. It is also safe to use concurrently from multiple
> +//! threads.
> +//!
> +//! It is different from the standard library's [`Arc`] in a few ways:
> +//! 1. It is backed by the kernel's `refcount_t` type.
> +//! 2. It does not support weak references, which allows it to be half the size.
> +//! 3. It saturates the reference count instead of aborting when it goes over a threshold.
> +//! 4. It does not provide a `get_mut` method, so the ref counted object is pinned.
> +//!
> +//! [`Arc`]: https://doc.rust-lang.org/std/sync/struct.Arc.html
> +
> +use crate::{bindings, error::Result, types::Opaque};
> +use alloc::boxed::Box;
> +use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
> +
> +/// A reference-counted pointer to an instance of `T`.
> +///
> +/// The reference count is incremented when new instances of [`Arc`] are created, and decremented
> +/// when they are dropped. When the count reaches zero, the underlying `T` is also dropped.
> +///
> +/// # Invariants
> +///
> +/// The reference count on an instance of [`Arc`] is always non-zero.
> +/// The object pointed to by [`Arc`] is always pinned.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::Arc;
> +///
> +/// struct Example {
> +/// a: u32,
> +/// b: u32,
> +/// }
> +///
> +/// // Create a ref-counted instance of `Example`.
> +/// let obj = Arc::try_new(Example { a: 10, b: 20 })?;
> +///
> +/// // Get a new pointer to `obj` and increment the refcount.
> +/// let cloned = obj.clone();
> +///
> +/// // Assert that both `obj` and `cloned` point to the same underlying object.
> +/// assert!(core::ptr::eq(&*obj, &*cloned));
> +///
> +/// // Destroy `obj` and decrement its refcount.
> +/// drop(obj);
> +///
> +/// // Check that the values are still accessible through `cloned`.
> +/// assert_eq!(cloned.a, 10);
> +/// assert_eq!(cloned.b, 20);
> +///
> +/// // The refcount drops to zero when `cloned` goes out of scope, and the memory is freed.
> +/// ```
> +pub struct Arc<T: ?Sized> {
> + ptr: NonNull<ArcInner<T>>,
> + _p: PhantomData<ArcInner<T>>,
> +}
> +
> +#[repr(C)]
> +struct ArcInner<T: ?Sized> {
> + refcount: Opaque<bindings::refcount_t>,
> + data: T,
> +}
> +
> +// SAFETY: It is safe to send `Arc<T>` to another thread when the underlying `T` is `Sync` because
> +// it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
> +// `T` to be `Send` because any thread that has an `Arc<T>` may ultimately access `T` directly, for
> +// example, when the reference count reaches zero and `T` is dropped.
> +unsafe impl<T: ?Sized + Sync + Send> Send for Arc<T> {}
> +
> +// SAFETY: It is safe to send `&Arc<T>` to another thread when the underlying `T` is `Sync` for the
> +// same reason as above. `T` needs to be `Send` as well because a thread can clone an `&Arc<T>`
> +// into an `Arc<T>`, which may lead to `T` being accessed by the same reasoning as above.
> +unsafe impl<T: ?Sized + Sync + Send> Sync for Arc<T> {}
> +
> +impl<T> Arc<T> {
> + /// Constructs a new reference counted instance of `T`.
> + pub fn try_new(contents: T) -> Result<Self> {
> + // INVARIANT: The refcount is initialised to a non-zero value.
> + let value = ArcInner {
> + // SAFETY: There are no safety requirements for this FFI call.
> + refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
> + data: contents,
> + };
> +
> + let inner = Box::try_new(value)?;
> +
> + // SAFETY: We just created `inner` with a reference count of 1, which is owned by the new
> + // `Arc` object.
> + Ok(unsafe { Self::from_inner(Box::leak(inner).into()) })
> + }
> +}
> +
> +impl<T: ?Sized> Arc<T> {
> + /// Constructs a new [`Arc`] from an existing [`ArcInner`].
> + ///
> + /// # Safety
> + ///
> + /// The caller must ensure that `inner` points to a valid location and has a non-zero reference
> + /// count, one of which will be owned by the new [`Arc`] instance.
> + unsafe fn from_inner(inner: NonNull<ArcInner<T>>) -> Self {
> + // INVARIANT: By the safety requirements, the invariants hold.
> + Arc {
> + ptr: inner,
> + _p: PhantomData,
> + }
> + }
> +}
> +
> +impl<T: ?Sized> Deref for Arc<T> {
> + type Target = T;
> +
> + fn deref(&self) -> &Self::Target {
> + // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> + // safe to dereference it.
> + unsafe { &self.ptr.as_ref().data }
> + }
> +}
> +
> +impl<T: ?Sized> Clone for Arc<T> {
> + fn clone(&self) -> Self {
> + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot overflow to zero.
> + // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> + // safe to increment the refcount.
> + unsafe { bindings::refcount_inc(self.ptr.as_ref().refcount.get()) };
> +
> + // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
> + unsafe { Self::from_inner(self.ptr) }
> + }
> +}
> +
> +impl<T: ?Sized> Drop for Arc<T> {
> + fn drop(&mut self) {
> + // SAFETY: By the type invariant, there is necessarily a reference to the object. We cannot
> + // touch `refcount` after it's decremented to a non-zero value because another thread/CPU
> + // may concurrently decrement it to zero and free it. It is ok to have a raw pointer to
> + // freed/invalid memory as long as it is never dereferenced.
> + let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
> +
> + // INVARIANT: If the refcount reaches zero, there are no other instances of `Arc`, and
> + // this instance is being dropped, so the broken invariant is not observable.
> + // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
> + let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> + if is_zero {
> + // The count reached zero, we must free the memory.
> + //
> + // SAFETY: The pointer was initialised from the result of `Box::leak`.
> + unsafe { Box::from_raw(self.ptr.as_ptr()) };
> + }
> + }
> +}
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH 4/7] rust: sync: introduce `ArcBorrow`
From: Emilio Cobos Álvarez @ 2023-01-04 16:06 UTC (permalink / raw)
To: Wedson Almeida Filho, Gary Guo
Cc: rust-for-linux, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Björn Roy Baron, linux-kernel
In-Reply-To: <CANeycqrVsbNJ+A+A26LXkBezBNUHvnZU2Q3_whexCwwG5ZcgPQ@mail.gmail.com>
Sorry for the drive-by comment, but maybe it saves some work.
On 1/4/23 16:29, Wedson Almeida Filho wrote:
> On Sat, 31 Dec 2022 at 19:43, Gary Guo <gary@garyguo.net> wrote:
>>
>> On Wed, 28 Dec 2022 06:03:43 +0000
>> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>>
>>> This allows us to create references to a ref-counted allocation without
>>> double-indirection and that still allow us to increment the refcount to
>>> a new `Arc<T>`.
>>>
>>> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
>>> ---
>>> rust/kernel/sync.rs | 2 +-
>>> rust/kernel/sync/arc.rs | 97 +++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 98 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>>> index 39b379dd548f..5de03ea83ea1 100644
>>> --- a/rust/kernel/sync.rs
>>> +++ b/rust/kernel/sync.rs
>>> @@ -7,4 +7,4 @@
>>>
>>> mod arc;
>>>
>>> -pub use arc::Arc;
>>> +pub use arc::{Arc, ArcBorrow};
>>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>>> index dbc7596cc3ce..f68bfc02c81a 100644
>>> --- a/rust/kernel/sync/arc.rs
>>> +++ b/rust/kernel/sync/arc.rs
>>> @@ -19,6 +19,7 @@ use crate::{bindings, error::Result, types::Opaque};
>>> use alloc::boxed::Box;
>>> use core::{
>>> marker::{PhantomData, Unsize},
>>> + mem::ManuallyDrop,
>>> ops::Deref,
>>> ptr::NonNull,
>>> };
>>> @@ -164,6 +165,18 @@ impl<T: ?Sized> Arc<T> {
>>> _p: PhantomData,
>>> }
>>> }
>>> +
>>> + /// Returns an [`ArcBorrow`] from the given [`Arc`].
>>> + ///
>>> + /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
>>> + /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised.
>>> + #[inline]
>>> + pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> {
>>> + // SAFETY: The constraint that the lifetime of the shared reference must outlive that of
>>> + // the returned `ArcBorrow` ensures that the object remains alive and that no mutable
>>> + // reference can be created.
>>> + unsafe { ArcBorrow::new(self.ptr) }
>>> + }
>>> }
>>>
>>> impl<T: ?Sized> Deref for Arc<T> {
>>> @@ -208,3 +221,87 @@ impl<T: ?Sized> Drop for Arc<T> {
>>> }
>>> }
>>> }
>>> +
>>> +/// A borrowed reference to an [`Arc`] instance.
>>> +///
>>> +/// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler
>>> +/// to use just `&T`, which we can trivially get from an `Arc<T>` instance.
>>> +///
>>> +/// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
>>> +/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
>>> +/// to a pointer (`Arc<T>`) to the object (`T`). An [`ArcBorrow`] eliminates this double
>>> +/// indirection while still allowing one to increment the refcount and getting an `Arc<T>` when/if
>>> +/// needed.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// There are no mutable references to the underlying [`Arc`], and it remains valid for the
>>> +/// lifetime of the [`ArcBorrow`] instance.
>>> +///
>>> +/// # Example
>>> +///
>>> +/// ```
>>> +/// use crate::sync::{Arc, ArcBorrow};
>>> +///
>>> +/// struct Example;
>>> +///
>>> +/// fn do_something(e: ArcBorrow<'_, Example>) -> Arc<Example> {
>>> +/// e.into()
>>> +/// }
>>> +///
>>> +/// let obj = Arc::try_new(Example)?;
>>> +/// let cloned = do_something(obj.as_arc_borrow());
>>> +///
>>> +/// // Assert that both `obj` and `cloned` point to the same underlying object.
>>> +/// assert!(core::ptr::eq(&*obj, &*cloned));
>>> +/// ```
>>> +pub struct ArcBorrow<'a, T: ?Sized + 'a> {
>>> + inner: NonNull<ArcInner<T>>,
>>> + _p: PhantomData<&'a ()>,
>>> +}
>>> +
>>> +impl<T: ?Sized> Clone for ArcBorrow<'_, T> {
>>> + fn clone(&self) -> Self {
>>> + *self
>>> + }
>>> +}
>>> +
>>> +impl<T: ?Sized> Copy for ArcBorrow<'_, T> {}
>>
>> Couldn't this just be derived `Clone` and `Copy`?
>
> Indeed. I'll send a v2 with this.
I'm not sure this is true. Deriving will add the T: Copy and T: Clone
bound, which I think is not what you want here.
i.e., I assume you want an ArcBorrow to be Copy even if the underlying T
is not.
See <https://github.com/rust-lang/rust/issues/26925> for the relevant
(really long-standing) Rust issue.
Cheers,
-- Emilio
^ permalink raw reply
* [PATCH] scripts: avoid redundant re-reading makefiles.
From: Alexander Pantyukhin @ 2023-01-04 16:04 UTC (permalink / raw)
To: ojeda, rust-for-linux, akpm; +Cc: alexpantyukhin
From: alexpantyukhin <apantykhin@gmail.com>
The main goal of the patch is avoid redundant re-reading makefiles
if these were read before. This is performance improvement.
Additional code improvements are using 'with' when read file and check
if file exists.
Signed-off-by: Alexander Pantyukhin <apantykhin@gmail.com>
---
scripts/generate_rust_analyzer.py | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/scripts/generate_rust_analyzer.py b/scripts/generate_rust_analyzer.py
index ecc7ea9a4dcf..e949f32b8123 100755
--- a/scripts/generate_rust_analyzer.py
+++ b/scripts/generate_rust_analyzer.py
@@ -98,13 +98,25 @@ def generate_crates(srctree, objtree, sysroot_src):
# Then, the rest outside of `rust/`.
#
# We explicitly mention the top-level folders we want to cover.
+ makefile_path_content = {}
for folder in ("samples", "drivers"):
for path in (srctree / folder).rglob("*.rs"):
logging.info("Checking %s", path)
name = path.name.replace(".rs", "")
# Skip those that are not crate roots.
- if f"{name}.o" not in open(path.parent / "Makefile").read():
+ makefile_path = path.parent / "Makefile"
+
+ if makefile_path not in makefile_path_content:
+ if makefile_path.is_file():
+ with open(makefile_path) as makefile:
+ makefile_path_content[makefile_path] = makefile.read()
+
+ else:
+ # If file is missing we consider it as empty-content file.
+ makefile_path_content[makefile_path] = ""
+
+ if f"{name}.o" not in makefile_path_content[makefile_path]:
continue
logging.info("Adding %s", name)
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 4/7] rust: sync: introduce `ArcBorrow`
From: Wedson Almeida Filho @ 2023-01-04 15:29 UTC (permalink / raw)
To: Gary Guo
Cc: rust-for-linux, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Björn Roy Baron, linux-kernel
In-Reply-To: <20221231194352.55cf0a26.gary@garyguo.net>
On Sat, 31 Dec 2022 at 19:43, Gary Guo <gary@garyguo.net> wrote:
>
> On Wed, 28 Dec 2022 06:03:43 +0000
> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> > This allows us to create references to a ref-counted allocation without
> > double-indirection and that still allow us to increment the refcount to
> > a new `Arc<T>`.
> >
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > ---
> > rust/kernel/sync.rs | 2 +-
> > rust/kernel/sync/arc.rs | 97 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 98 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> > index 39b379dd548f..5de03ea83ea1 100644
> > --- a/rust/kernel/sync.rs
> > +++ b/rust/kernel/sync.rs
> > @@ -7,4 +7,4 @@
> >
> > mod arc;
> >
> > -pub use arc::Arc;
> > +pub use arc::{Arc, ArcBorrow};
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index dbc7596cc3ce..f68bfc02c81a 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -19,6 +19,7 @@ use crate::{bindings, error::Result, types::Opaque};
> > use alloc::boxed::Box;
> > use core::{
> > marker::{PhantomData, Unsize},
> > + mem::ManuallyDrop,
> > ops::Deref,
> > ptr::NonNull,
> > };
> > @@ -164,6 +165,18 @@ impl<T: ?Sized> Arc<T> {
> > _p: PhantomData,
> > }
> > }
> > +
> > + /// Returns an [`ArcBorrow`] from the given [`Arc`].
> > + ///
> > + /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
> > + /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised.
> > + #[inline]
> > + pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> {
> > + // SAFETY: The constraint that the lifetime of the shared reference must outlive that of
> > + // the returned `ArcBorrow` ensures that the object remains alive and that no mutable
> > + // reference can be created.
> > + unsafe { ArcBorrow::new(self.ptr) }
> > + }
> > }
> >
> > impl<T: ?Sized> Deref for Arc<T> {
> > @@ -208,3 +221,87 @@ impl<T: ?Sized> Drop for Arc<T> {
> > }
> > }
> > }
> > +
> > +/// A borrowed reference to an [`Arc`] instance.
> > +///
> > +/// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler
> > +/// to use just `&T`, which we can trivially get from an `Arc<T>` instance.
> > +///
> > +/// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
> > +/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
> > +/// to a pointer (`Arc<T>`) to the object (`T`). An [`ArcBorrow`] eliminates this double
> > +/// indirection while still allowing one to increment the refcount and getting an `Arc<T>` when/if
> > +/// needed.
> > +///
> > +/// # Invariants
> > +///
> > +/// There are no mutable references to the underlying [`Arc`], and it remains valid for the
> > +/// lifetime of the [`ArcBorrow`] instance.
> > +///
> > +/// # Example
> > +///
> > +/// ```
> > +/// use crate::sync::{Arc, ArcBorrow};
> > +///
> > +/// struct Example;
> > +///
> > +/// fn do_something(e: ArcBorrow<'_, Example>) -> Arc<Example> {
> > +/// e.into()
> > +/// }
> > +///
> > +/// let obj = Arc::try_new(Example)?;
> > +/// let cloned = do_something(obj.as_arc_borrow());
> > +///
> > +/// // Assert that both `obj` and `cloned` point to the same underlying object.
> > +/// assert!(core::ptr::eq(&*obj, &*cloned));
> > +/// ```
> > +pub struct ArcBorrow<'a, T: ?Sized + 'a> {
> > + inner: NonNull<ArcInner<T>>,
> > + _p: PhantomData<&'a ()>,
> > +}
> > +
> > +impl<T: ?Sized> Clone for ArcBorrow<'_, T> {
> > + fn clone(&self) -> Self {
> > + *self
> > + }
> > +}
> > +
> > +impl<T: ?Sized> Copy for ArcBorrow<'_, T> {}
>
> Couldn't this just be derived `Clone` and `Copy`?
Indeed. I'll send a v2 with this.
>
> > +
> > +impl<T: ?Sized> ArcBorrow<'_, T> {
> > + /// Creates a new [`ArcBorrow`] instance.
> > + ///
> > + /// # Safety
> > + ///
> > + /// Callers must ensure the following for the lifetime of the returned [`ArcBorrow`] instance:
> > + /// 1. That `inner` remains valid;
> > + /// 2. That no mutable references to `inner` are created.
> > + unsafe fn new(inner: NonNull<ArcInner<T>>) -> Self {
> > + // INVARIANT: The safety requirements guarantee the invariants.
> > + Self {
> > + inner,
> > + _p: PhantomData,
> > + }
> > + }
> > +}
> > +
> > +impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
> > + fn from(b: ArcBorrow<'_, T>) -> Self {
> > + // SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop`
> > + // guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the
> > + // increment.
> > + ManuallyDrop::new(unsafe { Arc::from_inner(b.inner) })
> > + .deref()
> > + .clone()
> > + }
> > +}
>
> It might be easier to follow if this is jsut `bindings::refcount_inc`
> followed by `Arc::from_inner`?
I'd prefer to keep the interactions with `refcount_t` in `Arc` only so
that we can more easily change it in the future if we so choose.
> > +
> > +impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
> > + type Target = T;
> > +
> > + fn deref(&self) -> &Self::Target {
> > + // SAFETY: By the type invariant, the underlying object is still alive with no mutable
> > + // references to it, so it is safe to create a shared reference.
> > + unsafe { &self.inner.as_ref().data }
> > + }
> > +}
^ permalink raw reply
* Re: [PATCH] scripts: store Makefiles in dictionary
From: Miguel Ojeda @ 2023-01-04 11:42 UTC (permalink / raw)
To: Alexander Pantyukhin; +Cc: ojeda, rust-for-linux, akpm
In-Reply-To: <CAPi66w-wG4vuOQzfYruHp7-+s1ex-snDougUkuW70Lrf-6gKhA@mail.gmail.com>
On Wed, Jan 4, 2023 at 12:09 PM Alexander Pantyukhin
<apantykhin@gmail.com> wrote:
>
> Probably we also have to check if the Makefile exists. Or we can get FileNotFoundError. Do you think it is reasonable? Or we can always be sure that the Makefile for the `parent` always exists.
On missing `Makefile`s, please see
https://github.com/Rust-for-Linux/linux/pull/883, which should
eventually arrive upstream.
Also https://github.com/Rust-for-Linux/linux/pull/914 is related to
this piece of code.
> Thank you for your feedback!
My pleasure!
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH] scripts: store Makefiles in dictionary
From: Miguel Ojeda @ 2023-01-03 23:23 UTC (permalink / raw)
To: alexpantyukhin; +Cc: ojeda, rust-for-linux, akpm
In-Reply-To: <20230103210219.5690-1-apantykhin@gmail.com>
On Tue, Jan 3, 2023 at 10:03 PM alexpantyukhin <apantykhin@gmail.com> wrote:
>
> Signed-off-by: alexpantyukhin <apantykhin@gmail.com>
Just to confirm: is this your real name? Most `Signed-off-by` lines
are first name + surname (typically capitalized) or written in
non-Latin alphabets. Of course, if your name is spelled as you wrote
it, then that is fine!
> Store Makefiles by paths in dictionary and use these if required again
I think you intended for this to be the commit message, but currently
it is after the `---`, so the commit message is empty (except for the
`Signed-off-by`).
In any case, the commit message should explain why a change is made.
For instance, is this approach faster/simpler/... than the current
one? Or, in other words, what was the reason behind the change? (I
guess it is meant to make it faster, since you are caching the
contents, but by how much etc.?)
Thanks!
Cheers,
Miguel
^ permalink raw reply
* [PATCH] scripts: store Makefiles in dictionary
From: alexpantyukhin @ 2023-01-03 21:02 UTC (permalink / raw)
To: ojeda, rust-for-linux, akpm; +Cc: alexpantyukhin
Signed-off-by: alexpantyukhin <apantykhin@gmail.com>
---
Store Makefiles by paths in dictionary and use these if required again
scripts/generate_rust_analyzer.py | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/scripts/generate_rust_analyzer.py b/scripts/generate_rust_analyzer.py
index ecc7ea9a4dcf..9fafa0397e8b 100755
--- a/scripts/generate_rust_analyzer.py
+++ b/scripts/generate_rust_analyzer.py
@@ -98,13 +98,19 @@ def generate_crates(srctree, objtree, sysroot_src):
# Then, the rest outside of `rust/`.
#
# We explicitly mention the top-level folders we want to cover.
+ makefiles_path_content = {}
for folder in ("samples", "drivers"):
for path in (srctree / folder).rglob("*.rs"):
logging.info("Checking %s", path)
name = path.name.replace(".rs", "")
# Skip those that are not crate roots.
- if f"{name}.o" not in open(path.parent / "Makefile").read():
+ makefile_path = path.parent / "Makefile"
+ if makefile_path not in makefiles_path_content:
+ with open(makefile_path) as makefile:
+ makefiles_path_content[makefile_path] = makefile.read()
+
+ if f"{name}.o" not in makefiles_path_content[makefile_path]:
continue
logging.info("Adding %s", name)
--
2.25.1
^ permalink raw reply related
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