Hi everyone,
I believe kernfs_notify() poll latency can be improved if the poll
notification is done from kernfs_notify() directly rather than scheduled
work context.
I am sure there are good reasons why the fsnotify notification must be
done from scheduled work context (an obvious one is that it needs to be
able to sleep). But I don't see any reason why the poll notification
could not be done from kernfs_notify(). If there is any, please point it
out - I would highly appreciate it.
I came across this while working on a project that uses the sysfs GPIO
interface to wake a (real time) user space project on GPIO interrupts. I
know that interface is deprecated, but I still believe it's a valid
scenario and may occur with other drivers as well (current or future).
The sysfs GPIO interface (drivers/gpio/gpiolib-sysfs.c) interrupt
handler relies on kernfs_notify() (actually sysfs_notify_dirent(), but
that's just an alias) to wake any thread that may be poll()ing on the
interrupt. It is important to wake the thread as quickly as possible and
going through the kernel worker to handle the scheduled work is much
slower. Since the kernel worker runs with normal priority, this can even
become a case of priority inversion. If a higher priority thread hogs
the CPU, it may delay the kernel worker and in turn the thread that
needs to be notified (which could be a real time thread).
Best regards,
Radu Rendec
Radu Rendec (1):
Improve kernfs_notify() poll notification latency
fs/kernfs/file.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
--
2.17.2
kernfs_notify() does two notifications: poll and fsnotify. Originally,
both notifications were done from scheduled work context and all that
kernfs_notify() did was schedule the work.
This patch simply moves the poll notification from the scheduled work
handler to kernfs_notify(). The fsnotify notification still needs to be
done from scheduled work context because it can sleep (it needs to lock
a mutex).
If the poll notification is time critical (the notified thread needs to
wake as quickly as possible), it's better to do it from kernfs_notify()
directly. One example is calling sysfs_notify_dirent() from a hardware
interrupt handler to wake up a thread and handle the interrupt in user
space.
Signed-off-by: Radu Rendec <[email protected]>
---
fs/kernfs/file.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index dbf5bc250bfd..f8d5021a652e 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -857,7 +857,6 @@ static __poll_t kernfs_fop_poll(struct file *filp, poll_table *wait)
static void kernfs_notify_workfn(struct work_struct *work)
{
struct kernfs_node *kn;
- struct kernfs_open_node *on;
struct kernfs_super_info *info;
repeat:
/* pop one off the notify_list */
@@ -871,17 +870,6 @@ static void kernfs_notify_workfn(struct work_struct *work)
kn->attr.notify_next = NULL;
spin_unlock_irq(&kernfs_notify_lock);
- /* kick poll */
- spin_lock_irq(&kernfs_open_node_lock);
-
- on = kn->attr.open;
- if (on) {
- atomic_inc(&on->event);
- wake_up_interruptible(&on->poll);
- }
-
- spin_unlock_irq(&kernfs_open_node_lock);
-
/* kick fsnotify */
mutex_lock(&kernfs_mutex);
@@ -934,10 +922,21 @@ void kernfs_notify(struct kernfs_node *kn)
{
static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn);
unsigned long flags;
+ struct kernfs_open_node *on;
if (WARN_ON(kernfs_type(kn) != KERNFS_FILE))
return;
+ /* kick poll immediately */
+ spin_lock_irqsave(&kernfs_open_node_lock, flags);
+ on = kn->attr.open;
+ if (on) {
+ atomic_inc(&on->event);
+ wake_up_interruptible(&on->poll);
+ }
+ spin_unlock_irqrestore(&kernfs_open_node_lock, flags);
+
+ /* schedule work to kick fsnotify */
spin_lock_irqsave(&kernfs_notify_lock, flags);
if (!kn->attr.notify_next) {
kernfs_get(kn);
--
2.17.2
On Thu, Nov 15, 2018 at 09:09:54PM -0500, Radu Rendec wrote:
> kernfs_notify() does two notifications: poll and fsnotify. Originally,
> both notifications were done from scheduled work context and all that
> kernfs_notify() did was schedule the work.
>
> This patch simply moves the poll notification from the scheduled work
> handler to kernfs_notify(). The fsnotify notification still needs to be
> done from scheduled work context because it can sleep (it needs to lock
> a mutex).
>
> If the poll notification is time critical (the notified thread needs to
> wake as quickly as possible), it's better to do it from kernfs_notify()
> directly. One example is calling sysfs_notify_dirent() from a hardware
> interrupt handler to wake up a thread and handle the interrupt in user
> space.
>
> Signed-off-by: Radu Rendec <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Greg, can you please route this one?
Thanks.
--
tejun
On Tue, Nov 20, 2018 at 08:14:19AM -0800, Tejun Heo wrote:
> On Thu, Nov 15, 2018 at 09:09:54PM -0500, Radu Rendec wrote:
> > kernfs_notify() does two notifications: poll and fsnotify. Originally,
> > both notifications were done from scheduled work context and all that
> > kernfs_notify() did was schedule the work.
> >
> > This patch simply moves the poll notification from the scheduled work
> > handler to kernfs_notify(). The fsnotify notification still needs to be
> > done from scheduled work context because it can sleep (it needs to lock
> > a mutex).
> >
> > If the poll notification is time critical (the notified thread needs to
> > wake as quickly as possible), it's better to do it from kernfs_notify()
> > directly. One example is calling sysfs_notify_dirent() from a hardware
> > interrupt handler to wake up a thread and handle the interrupt in user
> > space.
> >
> > Signed-off-by: Radu Rendec <[email protected]>
>
> Acked-by: Tejun Heo <[email protected]>
>
> Greg, can you please route this one?
Sure, I'll queue it up soon...
greg k-h