On Tue, Dec 12, 2023 at 9:57 PM Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
>
> On Mon, Dec 11, 2023 at 05:25:18PM -0800, Boqun Feng wrote:
> > On Mon, Dec 11, 2023 at 09:41:28AM -0800, Boqun Feng wrote:
> > > On Mon, Dec 11, 2023 at 03:34:40PM +0000, Alice Ryhl wrote:
> > > > Benno Lossin <benno.lossin@xxxxxxxxx> writes:
> > > > > On 12/6/23 12:59, Alice Ryhl wrote:
> > > > > > + /// Schedule a task work that closes the file descriptor when this task returns to userspace.
> > > > > > + ///
> > > > > > + /// Fails if this is called from a context where we cannot run work when returning to
> > > > > > + /// userspace. (E.g., from a kthread.)
> > > > > > + pub fn close_fd(self, fd: u32) -> Result<(), DeferredFdCloseError> {
> > > > > > + use bindings::task_work_notify_mode_TWA_RESUME as TWA_RESUME;
> > > > > > +
> > > > > > + // In this method, we schedule the task work before closing the file. This is because
> > > > > > + // scheduling a task work is fallible, and we need to know whether it will fail before we
> > > > > > + // attempt to close the file.
> > > > > > +
> > > > > > + // SAFETY: Getting a pointer to current is always safe.
> > > > > > + let current = unsafe { bindings::get_current() };
> > > > > > +
> > > > > > + // SAFETY: Accessing the `flags` field of `current` is always safe.
> > > > > > + let is_kthread = (unsafe { (*current).flags } & bindings::PF_KTHREAD) != 0;
> > > > >
> > > > > Since Boqun brought to my attention that we already have a wrapper for
> > > > > `get_current()`, how about you use it here as well?
> > > >
> > > > I can use the wrapper, but it seems simpler to not go through a
> > > > reference when we just need a raw pointer.
> > > >
> > > > Perhaps we should have a safe `Task::current_raw` function that just
> > > > returns a raw pointer? It can still be safe.
> > > >
> > >
> > > I think we can have a `as_ptr` function for `Task`?
> > >
> > > impl Task {
> > > pub fn as_ptr(&self) -> *mut bindings::task_struct {
> > > self.0.get()
> > > }
> > > }
> >
> > Forgot mention, yes a ptr->ref->ptr trip may not be ideal, but I think
> > eventually we will have a task work wrapper, in that case maybe
> > Task::as_ptr() is still needed somehow.
> >
>
> After some more thoughts, I agree `Task::current_raw` may be better for
> the current usage, since we can also use it to wrap a
> `current_is_kthread` method like:
>
> impl Task {
> pub fn current_is_kthread() -> bool {
> let current = Self::current_raw();
>
> unsafe { (*current).flags & bindings::PF_KTHREAD != 0 }
> }
> }
I'll introduce a current_raw, then.
Alice
