-
Notifications
You must be signed in to change notification settings - Fork 51
rust: factory: Add rros_release_element function #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The compilation finishes with 13 warnings. All of them are about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
Overall, IMO it's better to use MaybeUninit inside the factory if we like C-style initialization.And we can use a factoryBuilder when initializing the factory. However, It seems that it needs a lot of designs and maybe we can put this schedule in the next refactoring.
| extern "C" { | ||
| fn rust_helper_hard_irqs_disabled() -> bool; | ||
| } | ||
| if running_inband().is_err() || unsafe { rust_helper_hard_irqs_disabled() } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why running_inband().is_err() instead of something like running_oob()? It's a little confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the built-in function unlikely if this condition rare happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
running_inband().is_err()instead of something likerunning_oob()? It's a little confusing
I don't find the function running_oob in our kernel. I can implement it and use it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the built-in function unlikely if this condition rare happens.
Thanks for your tips, using core::intrinsics::unlikely is better, it is already been added in core crate at rust-for-linux, I didn't know it before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I mean the API of running_inband here is very strange.
pub fn running_inband() -> Result<usize> {
let res = unsafe { rust_helper_running_inband() };
if res == 1 {
return Ok(0);
}
Err(Error::EINVAL)
}
I search for the running_inband method:
static __always_inline bool running_inband(void)
{
return stage_level() == 0;
}
static __always_inline bool running_oob(void)
{
return !running_inband();
}
I think we don not have to handle the error and just return true or false is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I mean the API of running_inband here is very strange.
pub fn running_inband() -> Result<usize> { let res = unsafe { rust_helper_running_inband() }; if res == 1 { return Ok(0); } Err(Error::EINVAL) }I search for the running_inband method:
static __always_inline bool running_inband(void) { return stage_level() == 0; } static __always_inline bool running_oob(void) { return !running_inband(); }I think we don not have to handle the error and just return true or false is ok.
Ok, I will change the running_inband function and related code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, there are a lot of codes that use running_inband().is_ok/err() to determine the current stage, so I think it would be more appropriate to create a new issue and modify this function later.
@ruiqurm
| extern "C" { | ||
| fn rust_helper_synchronize_rcu(); | ||
| } | ||
| let fac = unsafe { &*(self.factory.locked_data().get()) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a comment here? It's a little weird that you get the data inside but do not want to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean fac hasn't been used? I call the method dispose in fac.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I mean it seems that the dispose is a read-only field, which does not need to be guarded by the lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue is concerned with initialization. If the dispose is set at the initialization, there is no need to guard it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an advice. Can we require the subsystem to set a dispose method at compile time like that in C? I am not sure if it is difficult to implement currently.
| rust_helper_synchronize_rcu(); | ||
| } | ||
|
|
||
| fac.dispose.unwrap()(self); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is dispose a mandatory field? Maybe we should provide a way to let every subsystem pass its function pointer at initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now Option<fn(&mut RrosElement)>, subsystems can change the fn inside at the factory initialization.
Diff:
1. In factory.rs:
- Change the field `inside` and `dispose` in struct `RrosFactory`.
- Implement methods `remove_element_device`, `is_public`,
`__do_put_element`, `do_put_element_work`, `do_put_element_irq`,
`do_put_element`, `put_element` for `RrosElement`
- Modify the code used `inside`, because type has changed from `Option<RrosFactoryInside>` to
`RrosFactoryInside`.
- Add function `unbind_file_to_element`
- Add function `rros_open_element` and `rros_release_element`
2. In sub factory(proxy/buf/poll,etc. .rs):
Change the static factory creation.
3. In device.rs: Implement `unregister` for `Device`
4. In type.rs: Implement `hash_del` for `HlistNode`
Status:
1. Compilation: Compile with clang-13.0.1 toolchain, `ARCH=arm64`,
use `rros_defconfig`.
2. Functional test: Haven't started yet.
3. Eliminate warnings in factory.rs, most of them are dead_code warnings.
Diff:
insideanddisposein structRrosFactory.remove_element_device,is_public,__do_put_element,do_put_element_work,do_put_element_irq,do_put_element,put_elementforRrosElementinside, because type has changed fromOption<RrosFactoryInside>toRrosFactoryInside.unbind_file_to_elementrros_open_elementandrros_release_elementunregisterforDevicehash_delforHlistNodeStatus:
ARCH=arm64, userros_defconfig.Related issue:
resolving #47