Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.
On Wed, Mar 10, 2021 at 6:51 PM Sebastian Andrzej Siewior wrote: > > I assumed you are going to apply it but I don't see it in -next as of > today. Is there anything I need to do? Ah, since you said you were posting a patch, I kept this in the waiting list. I will apply the latest one then. Cheers, Miguel
Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.
On 2021-02-16 21:21:07 [+0100], Miguel Ojeda wrote: … > It is not an order :-) i.e. don't feel pressured that you need to sign > off on the comment change -- I can submit the comment on my own later > on. I assumed you are going to apply it but I don't see it in -next as of today. Is there anything I need to do? > Cheers, > Miguel Sebastian
Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.
On Tue, Feb 16, 2021 at 7:26 PM Sebastian Andrzej Siewior wrote: > > That should be part of the commit message. You can always rewind to the > commit message that introduce something and check if the commit message > made sense or ignored a detail which made it wrong (or so). No, it shouldn't. Commit messages are used to justify changes in the past, but they shouldn't be used as a replacement for documenting the present. The reason `in_interrupt()` is removed should be in the commit message; while the reason the precondition for `cond_resched()` is fulfilled should be in the code (assuming one finds the comment necessary, see below). > So it was needed once, it is not needed anymore. That was my arguing in > v1 about. No word about general removing in_interrupt() from drivers. Not sure what you mean by this. > This is not a fix. It just removes not needed code. Also I don't think It isn't a *bug* fix, yes, but it is a fix because the removal should have happened when the code was refactored. We can call it a cleanup, if you will. > that this is a good idea to add a comment to avoid someone to backtrack > / double check something. If you rely on a comment instead of double > checking that you do is indeed correct you will rely one day on a stale > comment and commit to a bug. Sure, comments and documentation may contain bugs like anything else. That does not mean comments and documentation are useless and we should remove all of them. In fact, the kernel lacks quite a lot of documentation. On the stale point: one-liner comments contiguous to the line they talk about and function docs are not as prone to get outdated as out-of-line docs like `Documentation/`. > To give you another example: If I would have come along and replaced > GFP_ATOMIC with GFP_KERNEL would you ask for a comment? Yes, I would, because if someone thought `GFP_ATOMIC` was needed at some point, then it is likely there is something non-obvious going on. Of course, it depends on the case. For instance, in the case of our patch, having a comment is not a big deal because it is fairly clear, specially for `charlcd_write()`. And `cond_resched()` has a `might_sleep()` annotation which helps catching future bugs. So if you don't add a comment, I will still take the patch since it is good patch. > Anyway, I'm posting a patch with changes as ordered in a jiffy. It is not an order :-) i.e. don't feel pressured that you need to sign off on the comment change -- I can submit the comment on my own later on. Cheers, Miguel
Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.
On 2021-02-16 13:42:19 [+0100], Miguel Ojeda wrote: > It is not so much about documenting the obvious, but about stating > that 1) the precondition was properly taken into account and that 2) > nothing non-obvious is undocumented. When code is changed later on, it > is much more likely assumptions are broken if not documented. That should be part of the commit message. You can always rewind to the commit message that introduce something and check if the commit message made sense or ignored a detail which made it wrong (or so). > In fact, from a quick git blame, that seems to be what happened here: > originally the function could be called from a public function > intended to be used from inside the kernel; so I assume it was the > intention to allow calls from softirq contexts. Then it was refactored > and the check never removed. In this case, the extra check is not a > big deal, but going in the opposite direction can happen too, and then > we will have a bug. So it was needed once, it is not needed anymore. That was my arguing in v1 about. No word about general removing in_interrupt() from drivers. > In general, when a patch for a fix is needed, it's usually a good idea > to add a comment right in the code. Even if only to avoid someone else > having to backtrack the calls to see it is only called form fs_ops > etc. This is not a fix. It just removes not needed code. Also I don't think that this is a good idea to add a comment to avoid someone to backtrack / double check something. If you rely on a comment instead of double checking that you do is indeed correct you will rely one day on a stale comment and commit to a bug. To give you another example: If I would have come along and replaced GFP_ATOMIC with GFP_KERNEL would you ask for a comment? Anyway, I'm posting a patch with changes as ordered in a jiffy. > Cheers, > Miguel Sebastian
Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.
On Tue, Feb 16, 2021 at 11:28 AM Sebastian Andrzej Siewior wrote: > > Could we please avoid documenting the obvious? It is more or less common > knowledge that the write callback (like any other) is preemptible user > context (in which write occurs). The same is true for register/probe > functions. The non-preemptible / atomic is mostly the exception because > of the callback. Like from a timer or an interrupt. It is not so much about documenting the obvious, but about stating that 1) the precondition was properly taken into account and that 2) nothing non-obvious is undocumented. When code is changed later on, it is much more likely assumptions are broken if not documented. In fact, from a quick git blame, that seems to be what happened here: originally the function could be called from a public function intended to be used from inside the kernel; so I assume it was the intention to allow calls from softirq contexts. Then it was refactored and the check never removed. In this case, the extra check is not a big deal, but going in the opposite direction can happen too, and then we will have a bug. In general, when a patch for a fix is needed, it's usually a good idea to add a comment right in the code. Even if only to avoid someone else having to backtrack the calls to see it is only called form fs_ops etc. Cheers, Miguel
Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.
On 2021-02-16 10:32:15 [+0100], Miguel Ojeda wrote: > Hi Sebastian, Hi, > On Sat, Feb 13, 2021 at 5:50 PM Sebastian Andrzej Siewior > wrote: > > > > charlcd_write() is invoked as a VFS->write() callback and as such it is > > always invoked from preemptible context and may sleep. > > Can we put this sentence as a comment in the code, right before the > call to cond_resched()? > > > charlcd_puts() is invoked from register/unregister callback which is > > preemtible. The reboot notifier callback is also invoked from > > Same for this one. Could we please avoid documenting the obvious? It is more or less common knowledge that the write callback (like any other) is preemptible user context (in which write occurs). The same is true for register/probe functions. The non-preemptible / atomic is mostly the exception because of the callback. Like from a timer or an interrupt. > In addition, somehow the spelling fixes got lost from the previous version. > > Same for the "code quotes": some have no quotes, others have `` or `'. > No big deal, I can fix it on my side if needed, but just letting you > know! :-) I'm so sorry. I must have taken the wrong patch while doing the update. My apologies. Once we sorted out the above, I will provide an update. > Thanks! > > Cheers, > Miguel Sebastian
Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.
Hi Sebastian, On Sat, Feb 13, 2021 at 5:50 PM Sebastian Andrzej Siewior wrote: > > charlcd_write() is invoked as a VFS->write() callback and as such it is > always invoked from preemptible context and may sleep. Can we put this sentence as a comment in the code, right before the call to cond_resched()? > charlcd_puts() is invoked from register/unregister callback which is > preemtible. The reboot notifier callback is also invoked from Same for this one. In addition, somehow the spelling fixes got lost from the previous version. Same for the "code quotes": some have no quotes, others have `` or `'. No big deal, I can fix it on my side if needed, but just letting you know! :-) Thanks! Cheers, Miguel
[PATCH v3] auxdisplay: Remove in_interrupt() usage.
charlcd_write() is invoked as a VFS->write() callback and as such it is
always invoked from preemptible context and may sleep.
charlcd_puts() is invoked from register/unregister callback which is
preemtible. The reboot notifier callback is also invoked from
preemptible context.
Therefore there is no need to use `in_interrupt()' to figure out if it
is save to sleep because it always is. `in_interrupt()` and related
context checks are being removed from non-core code.
Using `schedule()' to schedule (an be friendly to others) is
discouraged and `cond_resched()' should be used instead.
Remove `in_interrupt()' and use `cond_resched()' to schedule every 32
iteration if needed.
Link: https://lkml.kernel.org/r/20200914204209.256266...@linutronix.de
Cc: Miguel Ojeda Sandonis
Signed-off-by: Sebastian Andrzej Siewior
---
v2…v3: Extend the commit message as suggested by Miguel Ojeda.
v1…v2: Spelling fixes.
On 2021-02-10 22:46:23 [+0100], Miguel Ojeda wrote:
> Hi Sebastian,
Hi Miguel,
> Yeah, it is a bit confusing when reading without the context (it is
> hard to keep up with everything going on unless you work full-time on
> it :-)
Sorry for leaving it out. I though it is not needed since it was not
needed.
> > since this patch was small, simple and removing not required code I kept
> > it out. Is this enough information for you?
>
> If you don't mind, please add a quick sentence like (I can do it on my
> side too):
>
> `in_interrupt()` and related context checks are being removed
> from non-core code.
>
> Plus the tag:
>
> Link: https://lore.kernel.org/r/20200914204209.256266...@linutronix.de/
Added as suggested.
drivers/auxdisplay/charlcd.c | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index f43430e9dceed..fbfce95919f72 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -470,12 +470,8 @@ static ssize_t charlcd_write(struct file *file, const char
__user *buf,
char c;
for (; count-- > 0; (*ppos)++, tmp++) {
- if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
- /*
-* let's be a little nice with other processes
-* that need some CPU
-*/
- schedule();
+ if (((count + 1) & 0x1f) == 0)
+ cond_resched();
if (get_user(c, tmp))
return -EFAULT;
@@ -537,12 +533,8 @@ static void charlcd_puts(struct charlcd *lcd, const char
*s)
int count = strlen(s);
for (; count-- > 0; tmp++) {
- if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
- /*
-* let's be a little nice with other processes
-* that need some CPU
-*/
- schedule();
+ if (((count + 1) & 0x1f) == 0)
+ cond_resched();
charlcd_write_char(lcd, *tmp);
}
--
2.30.0
