# Meeting 2024-04-03
<!-- Leave your topic starting with ### in the relevant sections below -->

## Critical
<!-- bugs, soundness issues, urgent patches/reviews etc. -->

## Status Reports
<!-- You want to report on something you are working on/request reviews.
Or you want to know the status of something someone else is doing -->

### `syn` & friends in the kernel?

## Discussion Questions
<!-- Anything that requires lengthy discussion/more general questions also fit here -->

### Bikeshedding the name `ForeignBorrowed`

Context:

On 23.03.24 07:32, Andreas Hindborg wrote:
> Benno Lossin <[e-mail]> writes:
>> On 3/13/24 12:05, Andreas Hindborg wrote:
>>> +
>>> +    /// Data associated with the `struct request_queue` that is allocated for
>>> +    /// the `GenDisk` associated with this `Operations` implementation.
>>> +    type QueueData: ForeignOwnable;
>>> +
>>> +    /// Data associated with a dispatch queue. This is stored as a pointer in
>>> +    /// the C `struct blk_mq_hw_ctx` that represents a hardware queue.
>>> +    type HwData: ForeignOwnable;
>>> +
>>> +    /// Data associated with a `TagSet`. This is stored as a pointer in `struct
>>> +    /// blk_mq_tag_set`.
>>> +    type TagSetData: ForeignOwnable;
>>> +
>>> +    /// Called by the kernel to get an initializer for a `Pin<&mut RequestData>`.
>>> +    fn new_request_data(
>>> +        //rq: ARef<Request<Self>>,
>>> +        tagset_data: <Self::TagSetData as ForeignOwnable>::Borrowed<'_>,
>>
>> Since you need to use this pattern a lot, it might be a good idea to
>> introduce a type alias to help with this:
>>
>>       type ForeignBorrowed<'a, T> = <T as ForeignOwnable>::Borrowed<'a>;
>>
>> What do the others think?
>>
>> The function would then become (with the RPITIT improvement as well):
>>
>>       fn new_request_data(
>>           //rq: ARef<Request<Self>>,
>>           tagset_data: ForeignBorrowed<'_, Self::TagSetData>,
>>       ) -> impl PinInit<Self::RequestData>;
> 
> A bit more concise, I think it is better. I'll go ahead and kill that
> commented out argument that sneaked into the patch as well :)

I just picked `ForeignBorrowed`, since it was easy and quick, but we might want to choose a different one. Eg one that is shorter. Some options that came to mind:
- `Foreign`
- `FBorrow`

### Splitting up `helpers.c`

Anybody against splitting up `helpers.c` ?

https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Splitting.20up.20helpers.2Ec/near/430980731

## Miscellaneous
<!-- stuff that does not fit into other categories -->

### `u63` or `NonNegativeI64`

`ktime_t` in kernel (per Boqun's understanding):

* when used as a timestamp, the valid range is `[0, KTIME_MAX]`, where `KTIME_MAX` is `i64::MAX`.
* when used as a time delta, for the efficiency, the valid range is `[-KTIME_MAX, KTIME_MAX]`.

Therefore it's defined as an `i64`. Notice that I don't think negative timedeltas are the usual cases, they are probably used mostly for "rebasing" a duration, for example:

t2 = t1 + (base2 - base1);

most usages of a duration are adding it to a time base (e.g in timer code) to calculate a future timestamp. (You cannot arm a timer that fires in a past time).

so I (Boqun) generally agree Benno's suggestion [making Duration non negative](https://lore.kernel.org/rust-for-linux/kZmwObXVcyEJFVR3I05Nab0WdjcccDVARoOm6U9EmXhd89fxYRjvNmEAUZsueWTUlBlHp8jpU3i2YfgN9aWCLZameta0tPT_OgQdBOWUIko=@proton.me/), however to mirror the `ktime_t` semantics in kernel, I will need a `u63` or `NonNegativeI64` for `Duration`, so that `Option<Duration>` or `Either<Duration, Duration>` can be put into a `i64`. This will gives the same time and space efficiency as the C code.

My question is: does LLVM support this? If so, is there any Rust work already covering this?

Gary: Rust compiler does have a notion of scalar range, but it won't (and can't) optimise `Result<NonNegativeI64, NonNegativeI64>` (the reason being that you need ability to get `&mut NonNegativeI64` for both variants and it's not possible if there're only 8 bytes in total). `Option` works fine.

#![feature(rustc_attrs)]

#[repr(transparent)]
#[rustc_layout_scalar_valid_range_start(0)]
#[rustc_layout_scalar_valid_range_end(9_223_372_036_854_775_807)] // i64::MAX
pub struct NonNegativeI64(i64);

fn main() {
    println!("{}", core::mem::size_of::<Option<NonNegativeI64>>()); // 8
    println!("{}", core::mem::size_of::<Result<NonNegativeI64, NonNegativeI64>>()); // 16
}

I think we could have both a `Delta` type and a `Duration` type?

Cross ref to this discussion: https://internals.rust-lang.org/t/non-negative-integer-types/17796

fn elapsed_since(t1: TimeStamp, t2: TimeStamp) -> Option<Duration> {
    if t1 < t2 {
        return Some(t2 - t1);
    } else {
        return None;
    }
}    

Gary: I think what could be done is

impl Sub for TimeStamp {
    type Output = Delta;
}

impl Delta {
    fn as_duration(self) -> Option<Duration> {
        if self.0 < 0 {
            None
        } else {
            Some(self.0)
        }
    }
}

fn elapsed_since(t1: TimeStamp, t2: TimeStamp) -> Option<Duration> {
    (t1 - t2).as_duration()
}

If the `as_duration` part is inlined into the checking code then it'll just be optimised to checking delta is non-negative.