[v1] Devres optimization with bound devices

[PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

Posted by Danilo Krummrich 9 months, 2 weeks ago

For the I/O operations executed from the probe() method, take advantage
of Devres::access_with(), avoiding the atomic check and RCU read lock
required otherwise entirely.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 samples/rust/rust_driver_pci.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 9ce3a7323a16..3e1569e5096e 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
             GFP_KERNEL,
         )?;
 
-        let res = drvdata
-            .bar
-            .try_access_with(|b| Self::testdev(info, b))
-            .ok_or(ENXIO)??;
-
-        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
+        let bar = drvdata.bar.access_with(pdev.as_ref())?;
+        dev_info!(
+            pdev.as_ref(),
+            "pci-testdev data-match count: {}\n",
+            Self::testdev(info, bar)?
+        );
 
         Ok(drvdata.into())
     }
-- 
2.49.0

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

Posted by Benno Lossin 9 months, 2 weeks ago

On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> For the I/O operations executed from the probe() method, take advantage
> of Devres::access_with(), avoiding the atomic check and RCU read lock
> required otherwise entirely.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  samples/rust/rust_driver_pci.rs | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> index 9ce3a7323a16..3e1569e5096e 100644
> --- a/samples/rust/rust_driver_pci.rs
> +++ b/samples/rust/rust_driver_pci.rs
> @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
>              GFP_KERNEL,
>          )?;
>  
> -        let res = drvdata
> -            .bar
> -            .try_access_with(|b| Self::testdev(info, b))
> -            .ok_or(ENXIO)??;
> -
> -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
> +        let bar = drvdata.bar.access_with(pdev.as_ref())?;

Since this code might inspire other code, I don't think that we should
return `EINVAL` here (bubbled up from `access_with`). Not sure what the
correct thing here would be though...

---
Cheers,
Benno

> +        dev_info!(
> +            pdev.as_ref(),
> +            "pci-testdev data-match count: {}\n",
> +            Self::testdev(info, bar)?
> +        );
>  
>          Ok(drvdata.into())
>      }

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

Posted by Danilo Krummrich 9 months, 2 weeks ago

On Sat, Apr 26, 2025 at 08:30:39PM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> > For the I/O operations executed from the probe() method, take advantage
> > of Devres::access_with(), avoiding the atomic check and RCU read lock
> > required otherwise entirely.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  samples/rust/rust_driver_pci.rs | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> > index 9ce3a7323a16..3e1569e5096e 100644
> > --- a/samples/rust/rust_driver_pci.rs
> > +++ b/samples/rust/rust_driver_pci.rs
> > @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
> >              GFP_KERNEL,
> >          )?;
> >  
> > -        let res = drvdata
> > -            .bar
> > -            .try_access_with(|b| Self::testdev(info, b))
> > -            .ok_or(ENXIO)??;
> > -
> > -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
> > +        let bar = drvdata.bar.access_with(pdev.as_ref())?;
> 
> Since this code might inspire other code, I don't think that we should
> return `EINVAL` here (bubbled up from `access_with`). Not sure what the
> correct thing here would be though...

I can't think of any other error code that would match better, EINVAL seems to
be the correct thing. Maybe one could argue for ENODEV, but I still think EINVAL
fits better.

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

Posted by Benno Lossin 9 months, 2 weeks ago

On Sat Apr 26, 2025 at 11:26 PM CEST, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 08:30:39PM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> > For the I/O operations executed from the probe() method, take advantage
>> > of Devres::access_with(), avoiding the atomic check and RCU read lock
>> > required otherwise entirely.
>> >
>> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> > ---
>> >  samples/rust/rust_driver_pci.rs | 12 ++++++------
>> >  1 file changed, 6 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
>> > index 9ce3a7323a16..3e1569e5096e 100644
>> > --- a/samples/rust/rust_driver_pci.rs
>> > +++ b/samples/rust/rust_driver_pci.rs
>> > @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
>> >              GFP_KERNEL,
>> >          )?;
>> >  
>> > -        let res = drvdata
>> > -            .bar
>> > -            .try_access_with(|b| Self::testdev(info, b))
>> > -            .ok_or(ENXIO)??;
>> > -
>> > -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
>> > +        let bar = drvdata.bar.access_with(pdev.as_ref())?;
>> 
>> Since this code might inspire other code, I don't think that we should
>> return `EINVAL` here (bubbled up from `access_with`). Not sure what the
>> correct thing here would be though...
>
> I can't think of any other error code that would match better, EINVAL seems to
> be the correct thing. Maybe one could argue for ENODEV, but I still think EINVAL
> fits better.

The previous iteration of the sample used the ENXIO error code.

In this sample it should be impossible to trigger the error path. But
others might copy the code into a context where that is not the case and
then might have a horrible time debugging where the `EINVAL` came from.
I don't know if our answer to that should be "improve debugging errors
in general" or "improve the error handling in this case". I have no
idea how the former could look like, maybe something around
`#[track_caller]` and noting the lines where an error was created? For
the latter case, we could do:

    let bar = match drvdata.bar.access_with(pdev.as_ref()) {
        Ok(bar) => bar,
        Err(_) => {
            // `bar` was just created using the `pdev` above, so this should never happen.
            return Err(ENXIO);
        }
    };

---
Cheers,
Benno

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

Posted by Danilo Krummrich 9 months, 2 weeks ago

On Sun, Apr 27, 2025 at 08:56:29AM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 11:26 PM CEST, Danilo Krummrich wrote:
> > On Sat, Apr 26, 2025 at 08:30:39PM +0000, Benno Lossin wrote:
> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> >> > For the I/O operations executed from the probe() method, take advantage
> >> > of Devres::access_with(), avoiding the atomic check and RCU read lock
> >> > required otherwise entirely.
> >> >
> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> > ---
> >> >  samples/rust/rust_driver_pci.rs | 12 ++++++------
> >> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> >> > index 9ce3a7323a16..3e1569e5096e 100644
> >> > --- a/samples/rust/rust_driver_pci.rs
> >> > +++ b/samples/rust/rust_driver_pci.rs
> >> > @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
> >> >              GFP_KERNEL,
> >> >          )?;
> >> >  
> >> > -        let res = drvdata
> >> > -            .bar
> >> > -            .try_access_with(|b| Self::testdev(info, b))
> >> > -            .ok_or(ENXIO)??;
> >> > -
> >> > -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
> >> > +        let bar = drvdata.bar.access_with(pdev.as_ref())?;
> >> 
> >> Since this code might inspire other code, I don't think that we should
> >> return `EINVAL` here (bubbled up from `access_with`). Not sure what the
> >> correct thing here would be though...
> >
> > I can't think of any other error code that would match better, EINVAL seems to
> > be the correct thing. Maybe one could argue for ENODEV, but I still think EINVAL
> > fits better.
> 
> The previous iteration of the sample used the ENXIO error code.

Yes, because the cause of error for try_access_with() failing would have been
that the device was unbound already, hence a different error code.

> In this sample it should be impossible to trigger the error path. But
> others might copy the code into a context where that is not the case and
> then might have a horrible time debugging where the `EINVAL` came from.

I think it should always pretty unlikely design wise to supply a non-matching
device.

> I don't know if our answer to that should be "improve debugging errors
> in general" or "improve the error handling in this case". I have no
> idea how the former could look like, maybe something around
> `#[track_caller]` and noting the lines where an error was created? For
> the latter case, we could do:
> 
>     let bar = match drvdata.bar.access_with(pdev.as_ref()) {
>         Ok(bar) => bar,
>         Err(_) => {
>             // `bar` was just created using the `pdev` above, so this should never happen.
>             return Err(ENXIO);

If the caller really put in a non-matching device we can't say for sure that the
cause is ENXIO, the only thing we know is that the code author confused device
instances, so I think EINVAL is still the correct thing to return.

The problem that it might be difficult to figure out the source of the error
code has always been present in the kernel, and while I'm not saying we
shouldn't aim for improving this, I don't think this patch is quite the place
for this.

Re: [PATCH 3/3] samples: rust: pci: take advantage of Devres::access_with()

Posted by Benno Lossin 9 months, 2 weeks ago

On Sun Apr 27, 2025 at 12:20 PM CEST, Danilo Krummrich wrote:
> On Sun, Apr 27, 2025 at 08:56:29AM +0000, Benno Lossin wrote:
>> On Sat Apr 26, 2025 at 11:26 PM CEST, Danilo Krummrich wrote:
>> > On Sat, Apr 26, 2025 at 08:30:39PM +0000, Benno Lossin wrote:
>> >> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
>> >> > For the I/O operations executed from the probe() method, take advantage
>> >> > of Devres::access_with(), avoiding the atomic check and RCU read lock
>> >> > required otherwise entirely.
>> >> >
>> >> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> >> > ---
>> >> >  samples/rust/rust_driver_pci.rs | 12 ++++++------
>> >> >  1 file changed, 6 insertions(+), 6 deletions(-)
>> >> >
>> >> > diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
>> >> > index 9ce3a7323a16..3e1569e5096e 100644
>> >> > --- a/samples/rust/rust_driver_pci.rs
>> >> > +++ b/samples/rust/rust_driver_pci.rs
>> >> > @@ -83,12 +83,12 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
>> >> >              GFP_KERNEL,
>> >> >          )?;
>> >> >  
>> >> > -        let res = drvdata
>> >> > -            .bar
>> >> > -            .try_access_with(|b| Self::testdev(info, b))
>> >> > -            .ok_or(ENXIO)??;
>> >> > -
>> >> > -        dev_info!(pdev.as_ref(), "pci-testdev data-match count: {}\n", res);
>> >> > +        let bar = drvdata.bar.access_with(pdev.as_ref())?;
>> >> 
>> >> Since this code might inspire other code, I don't think that we should
>> >> return `EINVAL` here (bubbled up from `access_with`). Not sure what the
>> >> correct thing here would be though...
>> >
>> > I can't think of any other error code that would match better, EINVAL seems to
>> > be the correct thing. Maybe one could argue for ENODEV, but I still think EINVAL
>> > fits better.
>> 
>> The previous iteration of the sample used the ENXIO error code.
>
> Yes, because the cause of error for try_access_with() failing would have been
> that the device was unbound already, hence a different error code.

Ah I see.

>> In this sample it should be impossible to trigger the error path. But
>> others might copy the code into a context where that is not the case and
>> then might have a horrible time debugging where the `EINVAL` came from.
>
> I think it should always pretty unlikely design wise to supply a non-matching
> device.
>
>> I don't know if our answer to that should be "improve debugging errors
>> in general" or "improve the error handling in this case". I have no
>> idea how the former could look like, maybe something around
>> `#[track_caller]` and noting the lines where an error was created? For
>> the latter case, we could do:
>> 
>>     let bar = match drvdata.bar.access_with(pdev.as_ref()) {
>>         Ok(bar) => bar,
>>         Err(_) => {
>>             // `bar` was just created using the `pdev` above, so this should never happen.
>>             return Err(ENXIO);
>
> If the caller really put in a non-matching device we can't say for sure that the
> cause is ENXIO, the only thing we know is that the code author confused device
> instances, so I think EINVAL is still the correct thing to return.
>
> The problem that it might be difficult to figure out the source of the error
> code has always been present in the kernel, and while I'm not saying we
> shouldn't aim for improving this, I don't think this patch is quite the place
> for this.

Agreed.

---
Cheers,
Benno