| Message ID | 20200917120333.2337-1-ceggers@arri.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: trigger: Don't use RT priority | expand |
On Thu, 17 Sep 2020 14:03:33 +0200 Christian Eggers <ceggers@arri.de> wrote: > Triggers may raise transactions on slow busses like I2C. Using the > original RT priority of a threaded IRQ may prevent other important IRQ > handlers from being run. > > Signed-off-by: Christian Eggers <ceggers@arri.de> > Cc: stable@vger.kernel.org > --- > In my particular case (on a RT kernel), the RT priority of the sysfstrig > threaded IRQ handler caused (temporarily) raising the prio of a user > space process which was holding the I2C bus mutex. > > Due to a bug in the i2c-imx driver, this process spent 500 ms in a busy-wait > loop and prevented all threaded IRQ handlers from being run during this > time. I'm not sure I fully understand the impacts of this yet. What is the impact on cases where we don't have any nasty side affects due to users of the trigger? I presume reducing the priority will cause some reduction in performance? If so is there any chance that would count as a regression? Jonathan > > v2: > - Use sched_set_normal() instead of sched_setscheduler_nocheck() > > drivers/iio/industrialio-trigger.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c > index 6f16357fd732..7ed00ad695c7 100644 > --- a/drivers/iio/industrialio-trigger.c > +++ b/drivers/iio/industrialio-trigger.c > @@ -9,7 +9,10 @@ > #include <linux/err.h> > #include <linux/device.h> > #include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqdesc.h> > #include <linux/list.h> > +#include <linux/sched.h> > #include <linux/slab.h> > > #include <linux/iio/iio.h> > @@ -245,6 +248,7 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig, > int ret = 0; > bool notinuse > = bitmap_empty(trig->pool, CONFIG_IIO_CONSUMERS_PER_TRIGGER); > + struct irq_desc *irq_desc; > > /* Prevent the module from being removed whilst attached to a trigger */ > __module_get(pf->indio_dev->driver_module); > @@ -264,6 +268,12 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig, > if (ret < 0) > goto out_put_irq; > > + /* Triggers may raise transactions on slow busses like I2C. Using the original RT priority > + * of a threaded IRQ may prevent other threaded IRQ handlers from being run. > + */ > + irq_desc = irq_to_desc(pf->irq); > + sched_set_normal(irq_desc->action->thread, 0); > + > /* Enable trigger in driver */ > if (trig->ops && trig->ops->set_trigger_state && notinuse) { > ret = trig->ops->set_trigger_state(trig, true);
Hi Jonathan, On Thursday, 17 September 2020, 19:19:42 CEST, Jonathan Cameron wrote: > On Thu, 17 Sep 2020 14:03:33 +0200 Christian Eggers <ceggers@arri.de> wrote: > > Triggers may raise transactions on slow busses like I2C. Using the > > original RT priority of a threaded IRQ may prevent other important IRQ > > handlers from being run. > > > > Signed-off-by: Christian Eggers <ceggers@arri.de> > > Cc: stable@vger.kernel.org > > --- > > In my particular case (on a RT kernel), the RT priority of the sysfstrig > > threaded IRQ handler caused (temporarily) raising the prio of a user > > space process which was holding the I2C bus mutex. > > > > Due to a bug in the i2c-imx driver, this process spent 500 ms in a > > busy-wait loop and prevented all threaded IRQ handlers from being run > > during this time. > > I'm not sure I fully understand the impacts of this yet. > > What is the impact on cases where we don't have any nasty side affects > due to users of the trigger? The problem was not the user of the trigger. The problem was the (shared) resource (I2C bus) which the triggered iio driver uses. I would say that the i2c-imx driver is not "RT safe" [1]. This means that the driver performs busy-waiting, which is less a problem for normal priorities than for RT. If the busy-wait loop is run with RT prio, it will block everything else, even (threaded) interrupt handlers. > I presume reducing the priority will cause some reduction in > performance? If so is there any chance that would count as a regression? I expect that other user will complain if we do this, yes. But I would like to open the discussion, which priority is the "correct" one, or how this could be set up from user space. According to [2], there is not much value choosing the priority inside the kernel. Simply changing the priority of the trigger task using "chrt" seems difficult, as this can (currently) not be done of using libiio. > > Jonathan > > > v2: > > - Use sched_set_normal() instead of sched_setscheduler_nocheck() > > > > drivers/iio/industrialio-trigger.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > [1] https://lore.kernel.org/patchwork/cover/1307330/ [2] https://lwn.net/Articles/818388/
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index 6f16357fd732..7ed00ad695c7 100644 --- a/drivers/iio/industrialio-trigger.c +++ b/drivers/iio/industrialio-trigger.c @@ -9,7 +9,10 @@ #include <linux/err.h> #include <linux/device.h> #include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqdesc.h> #include <linux/list.h> +#include <linux/sched.h> #include <linux/slab.h> #include <linux/iio/iio.h> @@ -245,6 +248,7 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig, int ret = 0; bool notinuse = bitmap_empty(trig->pool, CONFIG_IIO_CONSUMERS_PER_TRIGGER); + struct irq_desc *irq_desc; /* Prevent the module from being removed whilst attached to a trigger */ __module_get(pf->indio_dev->driver_module); @@ -264,6 +268,12 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig, if (ret < 0) goto out_put_irq; + /* Triggers may raise transactions on slow busses like I2C. Using the original RT priority + * of a threaded IRQ may prevent other threaded IRQ handlers from being run. + */ + irq_desc = irq_to_desc(pf->irq); + sched_set_normal(irq_desc->action->thread, 0); + /* Enable trigger in driver */ if (trig->ops && trig->ops->set_trigger_state && notinuse) { ret = trig->ops->set_trigger_state(trig, true);
Triggers may raise transactions on slow busses like I2C. Using the original RT priority of a threaded IRQ may prevent other important IRQ handlers from being run. Signed-off-by: Christian Eggers <ceggers@arri.de> Cc: stable@vger.kernel.org --- In my particular case (on a RT kernel), the RT priority of the sysfstrig threaded IRQ handler caused (temporarily) raising the prio of a user space process which was holding the I2C bus mutex. Due to a bug in the i2c-imx driver, this process spent 500 ms in a busy-wait loop and prevented all threaded IRQ handlers from being run during this time. v2: - Use sched_set_normal() instead of sched_setscheduler_nocheck() drivers/iio/industrialio-trigger.c | 10 ++++++++++ 1 file changed, 10 insertions(+)