lkml.org 
[lkml]   [2021]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread()
On Sat, Oct 16, 2021 at 07:54:51PM +0200, Fabio M. De Francesco wrote:
> I guess that Dan will disagree with us :) Did you read his last message?
>
> I hope that he has time to review these patches. He expressed some doubts
> about splitting the changes in two separate patches. As far as I know, since
> Dan is a very experienced engineer (I am not even graduated and everything I
> know of Computer Science is self-taught), I could have been wrong in doing
> this work the way I did.
>

I did read it yes, he makes good points, and my motivation is simply
that the patches look fine as they are to me personally :-)

> > given that one semaphore was there for kthread start/stop and the other
> > for queuing. Looks good to me anyway based on what I know of completion
> > variables :-) I assume you've not made the waits killable or
> > interruptible in patch 1 due to the fact they are specifically related
> > to kthread start/stop?
>
> Good question! :)
>
> Let me explain how I chose to make one wait killable and the other
> uninterruptible.
>
> As far as I know, waiters may spin or sleep while waiting to acquire a lock
> (see spinlocks or mutexes for instance) or to be awakened (completions and
> condition variables for instance).
>
> These were the cases of sleeping waiters. Sleeping can be done in
> uninterruptible, interruptible / killable, and timed-out states.
>
> Where I'm 100% sure that the code doesn't require / want to be interrupted
> for whatever reason I prefer to use uninterruptible variants (and so I did in
> 1/3).
>
> When I'm not sure of the requirement above, I prefer to avoid that the
> process or the entire system hangs while waiting to acquire a mutex or to be
> awakened by a complete() (and so on).
>
> Conversely, using interruptible versions without proper checking of return
> codes and without proper managing of errors may lead to serious bugs.
>
> Kernel threads (kthreads) are like user processes / threads and are scheduled
> the same way the former are. One noteworthy difference is that their mm
> pointer is NULL (they have not an userspace address spaces). However they are
> still threads that have a PID in userspace and they can be killed by root.
>
> This is the output of the "ps -ef" command after "modprobe r8188eu":
>
> localhost:~ # ps -ef | grep RTW
> root 1726 2 0 19:06 ? 00:00:00 [RTW_CMD_THREAD]
>
> Since the developers who wrote the original code thought that that thread
> must be interrupted I thought that restricting interruptions to kills was the
> wiser choice in 2/3. Conversely, I cannot see reasons to interrupt the core
> part of a driver, so I chose to use an uninterruptible version of
> wait_for_completion*() in the other parts of the code.
>
> I warned you that I'm not an engineer, so please double check my argument :)
>

Sounds good to me, just wanted to know your reasoning.

> > Anyhow:
> >
> > For whole series:
> > Acked-by: Phillip Potter <phil@philpotter.co.uk>
>
> Thanks for you ack. I really appreciated it.
>

You're welcome :-)

Regards,
Phil

\
 
 \ /
  Last update: 2021-10-17 11:48    [W:0.043 / U:0.948 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site