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@xxxxxxxxxxxxxxxx> > > Thanks for you ack. I really appreciated it. > You're welcome :-) Regards, Phil
