Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Andreas Hindborg]

Hi Alexy,

Thanks for your patch. I think I understand why you would suggest the
change, with you strong C background. I would prefer that we do not
apply this change, see below.

Alexey Dobriyan <adobriyan@gmail.com> writes:

> On Sat, Aug 31, 2024 at 08:39:45PM +0000, Benno Lossin wrote:
>> On 31.08.24 22:15, Alexey Dobriyan wrote:
>> > Using range and contains() method is just fancy shmancy way of writing
>> 
>> This language doesn't fit into a commit message. Please give a technical
>> reason to change this.
>
> Oh come on!

Could you elaborate?

>
>> > two comparisons. Using range doesn't prevent any bugs here because
>> > typing "=" in range can be forgotten just as easily as in "<=" operator.
>> 
>> I don't think that using traditional comparisons is an improvement.
>
> They are an improvement, or rather contains() on integers is of dubious
> value.

I would disagree. To me, and probably to many people who are experienced
in Rust code, the range.contains() formulation is much more clear.

> First, coding style mandates that there are no whitespace on both sides
> of "..". This merges all characters into one Perl-like line noise.

I don't think it looks like noise or Perl. But I am not that experienced
in Perl 🤷

What code style are you referring to? We use `rustfmt` default settings
as code style, although I am not sure if that is written down anywhere.

> Second, when writing C I've a habit of writing comparisons like numeric
> line in school which goes from left to right:

But this is not C. In Rust we have other options.

>
> 	512 ... size .. PAGE_SIZE   ------> infinity
>
> See?
> Now it is easy to insert comparisons:
>
> 	512 <= size <= PAGE_SIZE
>
> Of course in C the middle variable must be duplicated but so what?
>
> How hard is to parse this?
>
> 	512 <= size && size <= PAGE_SIZE
>
>
> And thirdly, to a C/C++ dev, passing u32 by reference instead of by
> value to a function which obviously doesn't mutate it screams WHAT???

It might look a little funny, but in general lookups take references to
the key you are searching for. It makes sense for a larger set of types.
In this particular case, I don't think codegen is any different due to
the reference.

>
>> When
>> using `contains`, both of the bounds are visible with one look.
>
> Yes, they are within 4 characters of each other 2 of which are
> whitespace.

I like whitespace. I think it helps make the code more readable.


> This is what this patch is all about: contains() for integers?
> I can understand contains() instead of strstr() but for integers?

To me it makes sense to check if a number is in a range with `contains`.
I appreciate that it might not make sense to you, since it is not an
option in C.

>
>> When
>> using two comparisons, you have to first parse that they compare the
>> same variable and then look at the bounds.
>
> Yes but now you have to parse () and .. and &.

Reading Rust takes a bit of practice. Just like reading C takes some
practice to people who have not done it before.

>
>> > Also delete few comments of "increment i by 1" variety.
>> 
>> As Miguel already said, these are part of the documentation. Do not
>> remove them.
>
> Kernel has its fair share of 1:1 kernel-doc comments which contain
> exactly zero useful information because everything is in function
> signature already.

The comment is useful to a person browsing the documentation in the HTML
format. It is available here [1] if you want to have a look.


Best regards,
Andreas


[1] https://rust.docs.kernel.org/kernel/block/mq/gen_disk/struct.GenDiskBuilder.html#method.physical_block_size

Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Dmitry Torokhov]

Hi Andreas,

On Mon, Sep 02, 2024 at 09:57:17AM +0000, Andreas Hindborg wrote:
> 
> Hi Alexy,
> 
> Thanks for your patch. I think I understand why you would suggest the
> change, with you strong C background. I would prefer that we do not
> apply this change, see below.
> 
> Alexey Dobriyan <adobriyan@gmail.com> writes:
> 
> > On Sat, Aug 31, 2024 at 08:39:45PM +0000, Benno Lossin wrote:
> >> On 31.08.24 22:15, Alexey Dobriyan wrote:
> >> > Using range and contains() method is just fancy shmancy way of writing
> >> 
> >> This language doesn't fit into a commit message. Please give a technical
> >> reason to change this.
> >
> > Oh come on!
> 
> Could you elaborate?
> 
> >
> >> > two comparisons. Using range doesn't prevent any bugs here because
> >> > typing "=" in range can be forgotten just as easily as in "<=" operator.
> >> 
> >> I don't think that using traditional comparisons is an improvement.
> >
> > They are an improvement, or rather contains() on integers is of dubious
> > value.
> 
> I would disagree. To me, and probably to many people who are experienced
> in Rust code, the range.contains() formulation is much more clear.

If you want to keep dividing into Rust-land and C-land I'm afraid you
will have 2 islands that do not talk to each other. I really want to be
able to parse the things quickly and not constantly think if my Rust is
idiomatic enough or I could write the code in a more idiomatic way with
something brand new that just got off the nightly list and moved into
stable.

> 
> > First, coding style mandates that there are no whitespace on both sides
> > of "..". This merges all characters into one Perl-like line noise.
> 
> I don't think it looks like noise or Perl. But I am not that experienced
> in Perl 🤷
> 
> What code style are you referring to? We use `rustfmt` default settings
> as code style, although I am not sure if that is written down anywhere.
> 
> > Second, when writing C I've a habit of writing comparisons like numeric
> > line in school which goes from left to right:
> 
> But this is not C. In Rust we have other options.
> 
> >
> > 	512 ... size .. PAGE_SIZE   ------> infinity
> >
> > See?
> > Now it is easy to insert comparisons:
> >
> > 	512 <= size <= PAGE_SIZE
> >
> > Of course in C the middle variable must be duplicated but so what?
> >
> > How hard is to parse this?
> >
> > 	512 <= size && size <= PAGE_SIZE
> >
> >
> > And thirdly, to a C/C++ dev, passing u32 by reference instead of by
> > value to a function which obviously doesn't mutate it screams WHAT???
> 
> It might look a little funny, but in general lookups take references to
> the key you are searching for. It makes sense for a larger set of types.
> In this particular case, I don't think codegen is any different due to
> the reference.
> 
> >
> >> When
> >> using `contains`, both of the bounds are visible with one look.
> >
> > Yes, they are within 4 characters of each other 2 of which are
> > whitespace.
> 
> I like whitespace. I think it helps make the code more readable.
> 
> 
> > This is what this patch is all about: contains() for integers?
> > I can understand contains() instead of strstr() but for integers?
> 
> To me it makes sense to check if a number is in a range with `contains`.
> I appreciate that it might not make sense to you, since it is not an
> option in C.

I am sure we could come up with something like:

	if (!contains(MAKE_RANGE_INCL(512, 4096), size))
		...

but even if something possible it does not mean it needs to be done.

> 
> >
> >> When
> >> using two comparisons, you have to first parse that they compare the
> >> same variable and then look at the bounds.
> >
> > Yes but now you have to parse () and .. and &.
> 
> Reading Rust takes a bit of practice. Just like reading C takes some
> practice to people who have not done it before.
> 
> >
> >> > Also delete few comments of "increment i by 1" variety.
> >> 
> >> As Miguel already said, these are part of the documentation. Do not
> >> remove them.
> >
> > Kernel has its fair share of 1:1 kernel-doc comments which contain
> > exactly zero useful information because everything is in function
> > signature already.
> 
> The comment is useful to a person browsing the documentation in the HTML
> format. It is available here [1] if you want to have a look.

This is a private function and an implementation detail. Why does it
need to be exposed in documentation at all?

Thanks.

-- 
Dmitry

Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Miguel Ojeda]

On Tue, Sep 3, 2024 at 9:00 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> If you want to keep dividing into Rust-land and C-land I'm afraid you
> will have 2 islands that do not talk to each other. I really want to be

We are not trying to divide the Rust and C side, quite the opposite.
That should be obvious since dividing both sides only hurts the
project to begin with.

> able to parse the things quickly and not constantly think if my Rust is
> idiomatic enough or I could write the code in a more idiomatic way with
> something brand new that just got off the nightly list and moved into
> stable.

If a feature is in the minimum support version we have for Rust in the
kernel, and it improves the way we write code, then we should consider
taking advantage of it.

Now, that particular function call would have compiled since Rust 1.35
and ranges were already a concept back in Rust 1.0. So I am not sure
why you mention recently stabilized features here.

For this particular case, I don't think it matters too much, and I can
see arguments both ways (and we could introduce other ways to avoid
the reference or swap the order, e.g. `n.within(a..b)`).

> This is a private function and an implementation detail. Why does it
> need to be exposed in documentation at all?

That is a different question -- but even if it should be a private
function, it does not mean documentation should be removed (even if
currently we do not require documentation for private items).

Cheers,
Miguel

Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Dmitry Torokhov]

On Tue, Sep 03, 2024 at 09:30:53PM +0200, Miguel Ojeda wrote:
> On Tue, Sep 3, 2024 at 9:00 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > If you want to keep dividing into Rust-land and C-land I'm afraid you
> > will have 2 islands that do not talk to each other. I really want to be
> 
> We are not trying to divide the Rust and C side, quite the opposite.
> That should be obvious since dividing both sides only hurts the
> project to begin with.
> 
> > able to parse the things quickly and not constantly think if my Rust is
> > idiomatic enough or I could write the code in a more idiomatic way with
> > something brand new that just got off the nightly list and moved into
> > stable.
> 
> If a feature is in the minimum support version we have for Rust in the
> kernel, and it improves the way we write code, then we should consider
> taking advantage of it.
> 
> Now, that particular function call would have compiled since Rust 1.35
> and ranges were already a concept back in Rust 1.0. So I am not sure
> why you mention recently stabilized features here.

I was talking in general, not about this exact case, sorry if I was
unclear. But in general I personally have this issue with Rust and to
extent also with C++ where I constantly wonder if my code is "idiomatic
enough" or if it looks obsolete because it is "only" C++14 and not 17
or 20.

With C usually have no such concerns which allows me to concentrate on
different things.

> 
> For this particular case, I don't think it matters too much, and I can
> see arguments both ways (and we could introduce other ways to avoid
> the reference or swap the order, e.g. `n.within(a..b)`).
> 
> > This is a private function and an implementation detail. Why does it
> > need to be exposed in documentation at all?
> 
> That is a different question -- but even if it should be a private
> function, it does not mean documentation should be removed (even if
> currently we do not require documentation for private items).

I think exposing documentation for private function that can change at
any time and is not callable from outside has little value. That does
not mean that comments annotating such function have no value. But they
need to be taken together with the function code, and in this case
Alexey's concern about comments like "this increments that by 1"
becomes quite valid.

Thanks.

-- 
Dmitry

Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Benno Lossin]

On 03.09.24 21:47, Dmitry Torokhov wrote:
> On Tue, Sep 03, 2024 at 09:30:53PM +0200, Miguel Ojeda wrote:
>> On Tue, Sep 3, 2024 at 9:00 PM Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>> able to parse the things quickly and not constantly think if my Rust is
>>> idiomatic enough or I could write the code in a more idiomatic way with
>>> something brand new that just got off the nightly list and moved into
>>> stable.
>>
>> If a feature is in the minimum support version we have for Rust in the
>> kernel, and it improves the way we write code, then we should consider
>> taking advantage of it.
>>
>> Now, that particular function call would have compiled since Rust 1.35
>> and ranges were already a concept back in Rust 1.0. So I am not sure
>> why you mention recently stabilized features here.
> 
> I was talking in general, not about this exact case, sorry if I was
> unclear. But in general I personally have this issue with Rust and to
> extent also with C++ where I constantly wonder if my code is "idiomatic
> enough" or if it looks obsolete because it is "only" C++14 and not 17
> or 20.
> 
> With C usually have no such concerns which allows me to concentrate on
> different things.

I personally don't worry about whether my code is idiomatic. I just code
and when others give suggestions that sound good, I apply them. There is
nobody requiring all Rust code to be as idiomatic as possible. In the
review process you might be nudged to write in a more idiomatic style,
but that should also be true for C. The kernel is constantly adding new
ways of doing things and while the language itself doesn't evolve, the
codebase surely does.

>> For this particular case, I don't think it matters too much, and I can
>> see arguments both ways (and we could introduce other ways to avoid
>> the reference or swap the order, e.g. `n.within(a..b)`).
>>
>>> This is a private function and an implementation detail. Why does it
>>> need to be exposed in documentation at all?
>>
>> That is a different question -- but even if it should be a private
>> function, it does not mean documentation should be removed (even if
>> currently we do not require documentation for private items).
> 
> I think exposing documentation for private function that can change at
> any time and is not callable from outside has little value. That does
> not mean that comments annotating such function have no value. But they
> need to be taken together with the function code, and in this case
> Alexey's concern about comments like "this increments that by 1"
> becomes quite valid.

For the private `validate_block_size` function, I can see your argument.
However, I don't think that the function will change any time soon, so I
don't think it's a huge issue.

---
Cheers,
Benno

Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Miguel Ojeda]

On Tue, Sep 3, 2024 at 9:47 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> I was talking in general, not about this exact case, sorry if I was
> unclear. But in general I personally have this issue with Rust and to
> extent also with C++ where I constantly wonder if my code is "idiomatic
> enough" or if it looks obsolete because it is "only" C++14 and not 17
> or 20.
>
> With C usually have no such concerns which allows me to concentrate on
> different things.

Yeah, I see the concern, and I agree that C++ later standards can be
quite daunting to keep up with (and especially to learn new pitfalls
around UB in new features, e.g. C++ ranges).

With Rust, I think it is not that bad, at least so far and especially
for minor features -- generally they are well thought-out and fairly
regular, and at least for safe code the UB worry is not there, so it
is easier to feel confident in using them.

There are some major features, like `async`, that we may need to
carefully consider indeed though.

> I think exposing documentation for private function that can change at
> any time and is not callable from outside has little value. That does
> not mean that comments annotating such function have no value. But they
> need to be taken together with the function code, and in this case
> Alexey's concern about comments like "this increments that by 1"
> becomes quite valid.

To be clear, we are not exposing the documentation in the rendered
form for private items. It is possible to do so though, but we don't
enable that at the time being. Although I think it would be valuable
to have a "toggle" to show it.

I mean, sure, for private trivial functions, it may make not much
sense to add due to the burden of writing and maintaining it. That is
why we don't require documentation on private items so far, but we may
want to nevertheless in the future for particular core APIs (so we
could enable it in certain crates, for instance).

Thanks for taking a look, by the way.

Cheers,
Miguel

[PATCH RESEND] block, rust: simplify validate_block_size() function

[Alexey Dobriyan]

Using range and contains() method is just fancy shmancy way of writing
two comparisons. Using range doesn't prevent any bugs here because
typing "=" in range can be forgotten just as easily as in "<=" operator.

Also delete few comments of "increment i by 1" variety.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 rust/kernel/block/mq/gen_disk.rs |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -43,21 +43,16 @@ pub fn rotational(mut self, rotational: bool) -> Self {
         self
     }
 
-    /// Validate block size by verifying that it is between 512 and `PAGE_SIZE`,
-    /// and that it is a power of two.
     fn validate_block_size(size: u32) -> Result<()> {
-        if !(512..=bindings::PAGE_SIZE as u32).contains(&size) || !size.is_power_of_two() {
-            Err(error::code::EINVAL)
-        } else {
+        if 512 <= size && size <= bindings::PAGE_SIZE as u32 && size.is_power_of_two() {
             Ok(())
+        } else {
+            Err(error::code::EINVAL)
         }
     }
 
     /// Set the logical block size of the device to be built.
     ///
-    /// This method will check that block size is a power of two and between 512
-    /// and 4096. If not, an error is returned and the block size is not set.
-    ///
     /// This is the smallest unit the storage device can address. It is
     /// typically 4096 bytes.
     pub fn logical_block_size(mut self, block_size: u32) -> Result<Self> {
@@ -68,9 +63,6 @@ pub fn logical_block_size(mut self, block_size: u32) -> Result<Self> {
 
     /// Set the physical block size of the device to be built.
     ///
-    /// This method will check that block size is a power of two and between 512
-    /// and 4096. If not, an error is returned and the block size is not set.
-    ///
     /// This is the smallest unit a physical storage device can write
     /// atomically. It is usually the same as the logical block size but may be
     /// bigger. One example is SATA drives with 4096 byte physical block size

Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Benno Lossin]

On 31.08.24 22:15, Alexey Dobriyan wrote:
> Using range and contains() method is just fancy shmancy way of writing

This language doesn't fit into a commit message. Please give a technical
reason to change this.

> two comparisons. Using range doesn't prevent any bugs here because
> typing "=" in range can be forgotten just as easily as in "<=" operator.

I don't think that using traditional comparisons is an improvement. When
using `contains`, both of the bounds are visible with one look. When
using two comparisons, you have to first parse that they compare the
same variable and then look at the bounds.

> Also delete few comments of "increment i by 1" variety.

As Miguel already said, these are part of the documentation. Do not
remove them.

---
Cheers,
Benno

Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Alexey Dobriyan]

On Sat, Aug 31, 2024 at 08:39:45PM +0000, Benno Lossin wrote:
> On 31.08.24 22:15, Alexey Dobriyan wrote:
> > Using range and contains() method is just fancy shmancy way of writing
> 
> This language doesn't fit into a commit message. Please give a technical
> reason to change this.

Oh come on!

> > two comparisons. Using range doesn't prevent any bugs here because
> > typing "=" in range can be forgotten just as easily as in "<=" operator.
> 
> I don't think that using traditional comparisons is an improvement.

They are an improvement, or rather contains() on integers is of dubious
value.

First, coding style mandates that there are no whitespace on both sides
of "..". This merges all characters into one Perl-like line noise.

Second, when writing C I've a habit of writing comparisons like numeric
line in school which goes from left to right:

	512 ... size .. PAGE_SIZE   ------> infinity

See?
Now it is easy to insert comparisons:

	512 <= size <= PAGE_SIZE

Of course in C the middle variable must be duplicated but so what?

How hard is to parse this?

	512 <= size && size <= PAGE_SIZE


And thirdly, to a C/C++ dev, passing u32 by reference instead of by
value to a function which obviously doesn't mutate it screams WHAT???

> When
> using `contains`, both of the bounds are visible with one look.

Yes, they are within 4 characters of each other 2 of which are
whitespace.

This is what this patch is all about: contains() for integers?
I can understand contains() instead of strstr() but for integers?

> When
> using two comparisons, you have to first parse that they compare the
> same variable and then look at the bounds.

Yes but now you have to parse () and .. and &.

> > Also delete few comments of "increment i by 1" variety.
> 
> As Miguel already said, these are part of the documentation. Do not
> remove them.

Kernel has its fair share of 1:1 kernel-doc comments which contain
exactly zero useful information because everything is in function
signature already.

This is the original function:

	/* blk_validate_limits() validates bsize, so drivers don't usually need to */
	static inline int blk_validate_block_size(unsigned long bsize)
	{
	        if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
	                return -EINVAL;
	        return 0;
	}

I have to say this is useful comment because it refers to another more
complex function and hints that it should be used instead. It doesn't
reiterate what the function does internally.

Something was lost in translation.

Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Benno Lossin]

On 01.09.24 21:56, Alexey Dobriyan wrote:
> On Sat, Aug 31, 2024 at 08:39:45PM +0000, Benno Lossin wrote:
>> On 31.08.24 22:15, Alexey Dobriyan wrote:
>>> two comparisons. Using range doesn't prevent any bugs here because
>>> typing "=" in range can be forgotten just as easily as in "<=" operator.
>>
>> I don't think that using traditional comparisons is an improvement.
> 
> They are an improvement, or rather contains() on integers is of dubious
> value.
> 
> First, coding style mandates that there are no whitespace on both sides
> of "..". This merges all characters into one Perl-like line noise.

Can you please provide a link to that coding style? This is the default
formatting of rustfmt.

> Second, when writing C I've a habit of writing comparisons like numeric
> line in school which goes from left to right:
> 
> 	512 ... size .. PAGE_SIZE   ------> infinity
> 
> See?
> Now it is easy to insert comparisons:
> 
> 	512 <= size <= PAGE_SIZE
> 
> Of course in C the middle variable must be duplicated but so what?
> 
> How hard is to parse this?
> 
> 	512 <= size && size <= PAGE_SIZE

I would argue that this is just a matter of taste and familiarity. In
mathematics there are also several other ways to express the above.
For example $size \in [512, PAGE_SIZE]$.

> And thirdly, to a C/C++ dev, passing u32 by reference instead of by
> value to a function which obviously doesn't mutate it screams WHAT???

That is because the function is implemented with generics over the type
inside of the range. So if you have your own type, you can use `..` and
`..=` to create ranges. For big structs you wouldn't use by-value even
in C and C++.

>> When
>> using `contains`, both of the bounds are visible with one look.
> 
> Yes, they are within 4 characters of each other 2 of which are
> whitespace.
> 
> This is what this patch is all about: contains() for integers?
> I can understand contains() instead of strstr() but for integers?

Don't get me wrong, we can have a discussion about this. I just think
that it is not as clear-cut as you make it out to be.

>> When
>> using two comparisons, you have to first parse that they compare the
>> same variable and then look at the bounds.
> 
> Yes but now you have to parse () and .. and &.

As I said above, it's a matter of familiarity. I don't think that using
contains is inherently worse.

>>> Also delete few comments of "increment i by 1" variety.
>>
>> As Miguel already said, these are part of the documentation. Do not
>> remove them.
> 
> Kernel has its fair share of 1:1 kernel-doc comments which contain
> exactly zero useful information because everything is in function
> signature already.

Sure, but in this case, I don't think everything is in the function
signature.

> This is the original function:
> 
> 	/* blk_validate_limits() validates bsize, so drivers don't usually need to */
> 	static inline int blk_validate_block_size(unsigned long bsize)
> 	{
> 	        if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
> 	                return -EINVAL;
> 	        return 0;
> 	}
> 
> I have to say this is useful comment because it refers to another more
> complex function and hints that it should be used instead. It doesn't
> reiterate what the function does internally.
>
> Something was lost in translation.

We currently don't have that function in Rust. Also note that in
contrast to C, this function is private. So no driver is able to call it
anyways.

In this case, documentation is not really necessary, as we only require
it for publicly facing functions. But for those I think it is useful to
reiterate what the code does. They might be the entrypoint for first
time kernel developers and giving them an easier time of understanding
is IMO a good thing.

---
Cheers,
Benno