On 01.06.24 13:11, Andreas Hindborg wrote:
> Benno Lossin <benno.lossin@xxxxxxxxx> writes:
>> On 01.06.24 10:18, Andreas Hindborg wrote:
>>> + /// This function is called by the C kernel. A pointer to this function is
>>> + /// installed in the `blk_mq_ops` vtable for the driver.
>>> + ///
>>> + /// # Safety
>>> + ///
>>> + /// This function may only be called by blk-mq C infrastructure.
>>> + unsafe extern "C" fn commit_rqs_callback(_hctx: *mut bindings::blk_mq_hw_ctx) {
>>> + T::commit_rqs()
>>> + }
>>> +
>>> + /// This function is called by the C kernel. It is not currently
>>> + /// implemented, and there is no way to exercise this code path.
>>
>> Is it also possible to completely remove it? ie use `None` in the
>> VTABLE, or will the C side error?
>
> No, this pointer is not allowed to be null. It must be a callable
> function, hence the stub. It will be populated soon enough when I send
> patches for the remote completion logic.
Makes sense.
[...]
>>> +impl<T: Operations> TagSet<T> {
>>> + /// Try to create a new tag set
>>> + pub fn try_new(
>>> + nr_hw_queues: u32,
>>> + num_tags: u32,
>>> + num_maps: u32,
>>> + ) -> impl PinInit<Self, error::Error> {
>>> + try_pin_init!( TagSet {
>>> + // INVARIANT: We initialize `inner` here and it is valid after the
>>> + // initializer has run.
>>> + inner <- unsafe {kernel::init::pin_init_from_closure(move |place: *mut Opaque<bindings::blk_mq_tag_set>| -> Result<()> {
>>> + let place = place.cast::<bindings::blk_mq_tag_set>();
>>> +
>>> + // SAFETY: pin_init_from_closure promises that `place` is writable, and
>>> + // zeroes is a valid bit pattern for this structure.
>>> + core::ptr::write_bytes(place, 0, 1);
>>> +
>>> + /// For a raw pointer to a struct, write a struct field without
>>> + /// creating a reference to the field
>>> + macro_rules! write_ptr_field {
>>> + ($target:ident, $field:ident, $value:expr) => {
>>> + ::core::ptr::write(::core::ptr::addr_of_mut!((*$target).$field), $value)
>>> + };
>>> + }
>>> +
>>> + // SAFETY: pin_init_from_closure promises that `place` is writable
>>> + write_ptr_field!(place, ops, OperationsVTable::<T>::build());
>>> + write_ptr_field!(place, nr_hw_queues , nr_hw_queues);
>>> + write_ptr_field!(place, timeout , 0); // 0 means default which is 30 * HZ in C
>>> + write_ptr_field!(place, numa_node , bindings::NUMA_NO_NODE);
>>> + write_ptr_field!(place, queue_depth , num_tags);
>>> + write_ptr_field!(place, cmd_size , core::mem::size_of::<RequestDataWrapper>().try_into()?);
>>> + write_ptr_field!(place, flags , bindings::BLK_MQ_F_SHOULD_MERGE);
>>> + write_ptr_field!(place, driver_data , core::ptr::null_mut::<core::ffi::c_void>());
>>> + write_ptr_field!(place, nr_maps , num_maps);
>>
>> Did something not work with my suggestion?
>
> I did not want to change it if we are rewriting it to `Opaque::init`
> in a cycle or two, which I think is a better solution.
Ah I was suggesting to do it now, but emulate the `Opaque::init`
function (I should have been clearer about that).
I tried to do exactly that, but failed to easily implement it, since the
fields of `blk_mq_tag_set` include some structs, which of course do not
implement `Zeroable`.
Instead I came up with the following solution, which I find a lot nicer:
pub fn try_new(
nr_hw_queues: u32,
num_tags: u32,
num_maps: u32,
) -> impl PinInit<Self, error::Error> {
// SAFETY: `blk_mq_tag_set` only contains integers and pointers, which all are allowed to be 0.
let tag_set: bindings::blk_mq_tag_set = unsafe { core::mem::zeroed() };
let tag_set = bindings::blk_mq_tag_set {
ops: OperationsVTable::<T>::build(),
nr_hw_queues,
timeout: 0, // 0 means default which is 30Hz in C
numa_node: bindings::NUMA_NO_NODE,
queue_depth: num_tags,
cmd_size: core::mem::size_of::<RequestDataWrapper>().try_into()?,
flags: bindings::BLK_MQ_F_SHOULD_MERGE,
driver_data: core::ptr::null_mut::<core::ffi::c_void>(),
nr_maps: num_maps,
..tag_set
};
try_pin_init!(TagSet {
inner <- PinInit::<_, error::Error>::pin_chain(Opaque::new(tag_set), |tag_set| {
// SAFETY: we do not move out of `tag_set`.
let tag_set = unsafe { Pin::get_unchecked_mut(tag_set) };
// SAFETY: `tag_set` is a reference to an initialized `blk_mq_tag_set`.
error::to_result(unsafe { bindings::blk_mq_alloc_tag_set(tag_set) })
}),
_p: PhantomData,
})
}
I haven't tested it though, let me know if you have any problems.
---
Cheers,
Benno
