[PATCH linux dev-4.10] drivers: fsi: SBEFIFO: switch to workqueue instead of timer
Joel Stanley
joel at jms.id.au
Wed Dec 20 14:06:43 AEDT 2017
On Wed, Dec 20, 2017 at 1:27 PM, Joel Stanley <joel at jms.id.au> wrote:
> On Tue, Dec 19, 2017 at 6:43 AM, Eddie James <eajames at linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames at us.ibm.com>
>>
>> Some systems are experiencing scheduling problems; mod_timer can take up
>> to several seconds before the timer_function is executed, even if passed
>> the current jiffies.
>
> How did you work out that the timer wasn't firing?
>
> Can you describe the other tests you have done?
>
> It's not recommended to paper over something going wrong by changing
> over to a workqueue.
Nick pointed me to this fix, which was merged in 4.13:
2fe59f507a6 ("timers: Fix excessive granularity of new timers after a
nohz idle")
Can you cherry pick that into your tree and report the numbers that
you see before and after the patch?
>
>>
>> Work around this by using sbefifo's own workqueue.
>>
>> Signed-off-by: Edward A. James <eajames at us.ibm.com>
>> ---
>> drivers/fsi/fsi-sbefifo.c | 54 ++++++++++++++++++++++++++++-------------------
>> 1 file changed, 32 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index 0697a10..7b6842a 100644
>> --- a/drivers/fsi/fsi-sbefifo.c
>> +++ b/drivers/fsi/fsi-sbefifo.c
>> @@ -29,9 +29,9 @@
>> #include <linux/sched.h>
>> #include <linux/slab.h>
>> #include <linux/spinlock.h>
>> -#include <linux/timer.h>
>> #include <linux/uaccess.h>
>> #include <linux/wait.h>
>> +#include <linux/workqueue.h>
>>
>> /*
>> * The SBEFIFO is a pipe-like FSI device for communicating with
>> @@ -57,7 +57,7 @@
>> #define SBEFIFO_MAX_RESCHDULE msecs_to_jiffies(5000)
>>
>> struct sbefifo {
>> - struct timer_list poll_timer;
>> + struct delayed_work work;
>> struct fsi_device *fsi_dev;
>> struct miscdevice mdev;
>> wait_queue_head_t wait;
>> @@ -99,6 +99,8 @@ struct sbefifo_client {
>> unsigned long f_flags;
>> };
>>
>> +static struct workqueue_struct *sbefifo_wq;
>> +
>> static DEFINE_IDA(sbefifo_ida);
>>
>> static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
>> @@ -337,7 +339,7 @@ static void sbefifo_client_release(struct kref *kref)
>> */
>> set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>> sbefifo_get(sbefifo);
>> - if (mod_timer(&client->dev->poll_timer, jiffies))
>> + if (!queue_work(sbefifo_wq, &sbefifo->work.work))
>> sbefifo_put(sbefifo);
>> }
>> }
>> @@ -374,10 +376,11 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
>> return NULL;
>> }
>>
>> -static void sbefifo_poll_timer(unsigned long data)
>> +static void sbefifo_worker(struct work_struct *work)
>> {
>> static const unsigned long EOT_MASK = 0x000000ff;
>> - struct sbefifo *sbefifo = (void *)data;
>> + struct delayed_work *dwork = to_delayed_work(work);
>> + struct sbefifo *sbefifo = container_of(dwork, struct sbefifo, work);
>> struct sbefifo_buf *rbuf, *wbuf;
>> struct sbefifo_xfr *xfr, *tmp;
>> struct sbefifo_buf drain;
>> @@ -393,6 +396,7 @@ static void sbefifo_poll_timer(unsigned long data)
>> if (!xfr)
>> goto out_unlock;
>>
>> +again:
>> rbuf = xfr->rbuf;
>> wbuf = xfr->wbuf;
>>
>> @@ -414,9 +418,8 @@ static void sbefifo_poll_timer(unsigned long data)
>> devn = sbefifo_dev_nwwriteable(sts);
>> if (devn == 0) {
>> /* No open slot for write. Reschedule. */
>> - sbefifo->poll_timer.expires = jiffies +
>> - SBEFIFO_RESCHEDULE;
>> - add_timer(&sbefifo->poll_timer);
>> + queue_delayed_work(sbefifo_wq, &sbefifo->work,
>> + SBEFIFO_RESCHEDULE);
>> goto out_unlock;
>> }
>>
>> @@ -468,9 +471,8 @@ static void sbefifo_poll_timer(unsigned long data)
>> }
>>
>> /* No data yet. Reschedule. */
>> - sbefifo->poll_timer.expires = jiffies +
>> - SBEFIFO_RESCHEDULE;
>> - add_timer(&sbefifo->poll_timer);
>> + queue_delayed_work(sbefifo_wq, &sbefifo->work,
>> + SBEFIFO_RESCHEDULE);
>> goto out_unlock;
>> } else {
>> xfr->wait_data_timeout = 0;
>> @@ -516,10 +518,12 @@ static void sbefifo_poll_timer(unsigned long data)
>> }
>> INIT_LIST_HEAD(&sbefifo->xfrs);
>>
>> - } else if (eot && sbefifo_next_xfr(sbefifo)) {
>> - sbefifo_get(sbefifo);
>> - sbefifo->poll_timer.expires = jiffies;
>> - add_timer(&sbefifo->poll_timer);
>> + } else if (eot) {
>> + xfr = sbefifo_next_xfr(sbefifo);
>> + if (xfr) {
>> + wake_up_interruptible(&sbefifo->wait);
>> + goto again;
>> + }
>> }
>>
>> sbefifo_put(sbefifo);
>> @@ -638,7 +642,7 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>> * Fill the read buffer back up.
>> */
>> sbefifo_get(sbefifo);
>> - if (mod_timer(&client->dev->poll_timer, jiffies))
>> + if (!queue_work(sbefifo_wq, &sbefifo->work.work))
>> sbefifo_put(sbefifo);
>> } else {
>> list_del(&xfr->client);
>> @@ -721,7 +725,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>> &n))) {
>> set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>> sbefifo_get(sbefifo);
>> - if (mod_timer(&sbefifo->poll_timer, jiffies))
>> + if (!queue_work(sbefifo_wq, &sbefifo->work.work))
>> sbefifo_put(sbefifo);
>>
>> ret = -ERESTARTSYS;
>> @@ -741,7 +745,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>> n)) {
>> set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>> sbefifo_get(sbefifo);
>> - if (mod_timer(&sbefifo->poll_timer, jiffies))
>> + if (!queue_work(sbefifo_wq,
>> + &sbefifo->work.work))
>> sbefifo_put(sbefifo);
>> ret = -EFAULT;
>> goto out;
>> @@ -768,7 +773,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>> * Drain the write buffer.
>> */
>> sbefifo_get(sbefifo);
>> - if (mod_timer(&client->dev->poll_timer, jiffies))
>> + if (!queue_work(sbefifo_wq, &sbefifo->work.work))
>> sbefifo_put(sbefifo);
>> }
>>
>> @@ -928,8 +933,7 @@ static int sbefifo_probe(struct device *dev)
>> sbefifo->idx);
>>
>> /* This bit of silicon doesn't offer any interrupts... */
>> - setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
>> - (unsigned long)sbefifo);
>> + INIT_DELAYED_WORK(&sbefifo->work, sbefifo_worker);
>>
>> sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
>> sbefifo->mdev.fops = &sbefifo_fops;
>> @@ -982,7 +986,7 @@ static int sbefifo_remove(struct device *dev)
>>
>> ida_simple_remove(&sbefifo_ida, sbefifo->idx);
>>
>> - if (del_timer_sync(&sbefifo->poll_timer))
>> + if (cancel_delayed_work_sync(&sbefifo->work))
>> sbefifo_put(sbefifo);
>>
>> sbefifo_put(sbefifo);
>> @@ -1010,11 +1014,17 @@ static int sbefifo_remove(struct device *dev)
>>
>> static int sbefifo_init(void)
>> {
>> + sbefifo_wq = create_singlethread_workqueue("sbefifo");
>> + if (!sbefifo_wq)
>> + return -ENOMEM;
>> +
>> return fsi_driver_register(&sbefifo_drv);
>> }
>>
>> static void sbefifo_exit(void)
>> {
>> + destroy_workqueue(sbefifo_wq);
>> +
>> fsi_driver_unregister(&sbefifo_drv);
>>
>> ida_destroy(&sbefifo_ida);
>> --
>> 1.8.3.1
>>
More information about the openbmc
mailing list