| Message ID | 20200713204300.345975-1-badhri@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: tcpm: Move to high priority workqueue for processing events | expand |
On Mon, Jul 13, 2020 at 01:43:00PM -0700, Badhri Jagan Sridharan wrote: > "tReceiverResponse 15 ms Section 6.6.2 > The receiver of a Message requiring a response Shall respond > within tReceiverResponse in order to ensure that the > sender’s SenderResponseTimer does not expire." > > When the cpu complex is busy running other lower priority > work items, TCPM's work queue sometimes does not get scheduled > on time to meet the above requirement from the spec. > Elevating the TCPM's work queue to higher priority allows > TCPM to meet tReceiverResponse in a busy system. > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > --- > drivers/usb/typec/tcpm/tcpm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 82b19ebd7838e0..088b6f1fa1ff89 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -4747,7 +4747,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) > mutex_init(&port->lock); > mutex_init(&port->swap_lock); > > - port->wq = create_singlethread_workqueue(dev_name(dev)); > + port->wq = alloc_ordered_workqueue("%s", WQ_HIGHPRI, dev_name(dev)); How are you "guaranteeing" that this is really going to change anything on a highly loaded machine? Yes, it might make things better, but if you have a hard deadline like this, you need to do things a bit differently to always ensure that you meet it. I do not think this change is that fix, do you? thanks, greg k-h
On 7/13/20 11:05 PM, reg Kroah-Hartman wrote: > On Mon, Jul 13, 2020 at 01:43:00PM -0700, Badhri Jagan Sridharan wrote: >> "tReceiverResponse 15 ms Section 6.6.2 >> The receiver of a Message requiring a response Shall respond >> within tReceiverResponse in order to ensure that the >> sender’s SenderResponseTimer does not expire." >> >> When the cpu complex is busy running other lower priority >> work items, TCPM's work queue sometimes does not get scheduled >> on time to meet the above requirement from the spec. >> Elevating the TCPM's work queue to higher priority allows >> TCPM to meet tReceiverResponse in a busy system. >> >> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> >> --- >> drivers/usb/typec/tcpm/tcpm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >> index 82b19ebd7838e0..088b6f1fa1ff89 100644 >> --- a/drivers/usb/typec/tcpm/tcpm.c >> +++ b/drivers/usb/typec/tcpm/tcpm.c >> @@ -4747,7 +4747,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) >> mutex_init(&port->lock); >> mutex_init(&port->swap_lock); >> >> - port->wq = create_singlethread_workqueue(dev_name(dev)); >> + port->wq = alloc_ordered_workqueue("%s", WQ_HIGHPRI, dev_name(dev)); > > How are you "guaranteeing" that this is really going to change anything > on a highly loaded machine? > > Yes, it might make things better, but if you have a hard deadline like > this, you need to do things a bit differently to always ensure that you > meet it. I do not think this change is that fix, do you? > Good point. The worker in drivers/watchdog/watchdog_dev.c might be useful as a starting point. There may be better examples - this is just one I know of which had a similar problem. See commits 38a1222ae4f3 and 1ff688209e2e. Guenter
On Mon, Jul 13, 2020 at 11:58 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 7/13/20 11:05 PM, reg Kroah-Hartman wrote: > > On Mon, Jul 13, 2020 at 01:43:00PM -0700, Badhri Jagan Sridharan wrote: > >> "tReceiverResponse 15 ms Section 6.6.2 > >> The receiver of a Message requiring a response Shall respond > >> within tReceiverResponse in order to ensure that the > >> sender’s SenderResponseTimer does not expire." > >> > >> When the cpu complex is busy running other lower priority > >> work items, TCPM's work queue sometimes does not get scheduled > >> on time to meet the above requirement from the spec. > >> Elevating the TCPM's work queue to higher priority allows > >> TCPM to meet tReceiverResponse in a busy system. > >> > >> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > >> --- > >> drivers/usb/typec/tcpm/tcpm.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > >> index 82b19ebd7838e0..088b6f1fa1ff89 100644 > >> --- a/drivers/usb/typec/tcpm/tcpm.c > >> +++ b/drivers/usb/typec/tcpm/tcpm.c > >> @@ -4747,7 +4747,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) > >> mutex_init(&port->lock); > >> mutex_init(&port->swap_lock); > >> > >> - port->wq = create_singlethread_workqueue(dev_name(dev)); > >> + port->wq = alloc_ordered_workqueue("%s", WQ_HIGHPRI, dev_name(dev)); > > > > How are you "guaranteeing" that this is really going to change anything > > on a highly loaded machine? > > > > Yes, it might make things better, but if you have a hard deadline like > > this, you need to do things a bit differently to always ensure that you > > meet it. I do not think this change is that fix, do you? > > Yes Greg I agree with you, moving to HIGHPRI was making it better but is not going to solve the problem always. I was wondering whether are there better ways of doing this. > > Good point. The worker in drivers/watchdog/ !watchdog_dev.c might be > useful as a starting point. There may be better examples - this is > just one I know of which had a similar problem. See commits > 38a1222ae4f3 and 1ff688209e2e. > > Guenter Thanks a lot Guenter !! Very useful pointers, will review the approaches in both the commits !
Hi Guenter, Just sent out the patch "usb: typec: tcpm: Migrate workqueue to RT priority for processing events" which uses kthread_create_worker and hrtimer.nAppreciate your guidance !! The commits 38a1222ae4f3 and 1ff688209e2e were spot on as they were trying solve the same problem in a different subsystem. Thanks, Badhri On Tue, Jul 14, 2020 at 10:16 AM Badhri Jagan Sridharan <badhri@google.com> wrote: > > On Mon, Jul 13, 2020 at 11:58 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On 7/13/20 11:05 PM, reg Kroah-Hartman wrote: > > > On Mon, Jul 13, 2020 at 01:43:00PM -0700, Badhri Jagan Sridharan wrote: > > >> "tReceiverResponse 15 ms Section 6.6.2 > > >> The receiver of a Message requiring a response Shall respond > > >> within tReceiverResponse in order to ensure that the > > >> sender’s SenderResponseTimer does not expire." > > >> > > >> When the cpu complex is busy running other lower priority > > >> work items, TCPM's work queue sometimes does not get scheduled > > >> on time to meet the above requirement from the spec. > > >> Elevating the TCPM's work queue to higher priority allows > > >> TCPM to meet tReceiverResponse in a busy system. > > >> > > >> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > >> --- > > >> drivers/usb/typec/tcpm/tcpm.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > >> index 82b19ebd7838e0..088b6f1fa1ff89 100644 > > >> --- a/drivers/usb/typec/tcpm/tcpm.c > > >> +++ b/drivers/usb/typec/tcpm/tcpm.c > > >> @@ -4747,7 +4747,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) > > >> mutex_init(&port->lock); > > >> mutex_init(&port->swap_lock); > > >> > > >> - port->wq = create_singlethread_workqueue(dev_name(dev)); > > >> + port->wq = alloc_ordered_workqueue("%s", WQ_HIGHPRI, dev_name(dev)); > > > > > > How are you "guaranteeing" that this is really going to change anything > > > on a highly loaded machine? > > > > > > Yes, it might make things better, but if you have a hard deadline like > > > this, you need to do things a bit differently to always ensure that you > > > meet it. I do not think this change is that fix, do you? > > > > Yes Greg I agree with you, moving to HIGHPRI was making it better but > is not going to > solve the problem always. I was wondering whether are there better > ways of doing this. > > > > > Good point. The worker in drivers/watchdog/ !watchdog_dev.c might be > > useful as a starting point. There may be better examples - this is > > just one I know of which had a similar problem. See commits > > 38a1222ae4f3 and 1ff688209e2e. > > > > Guenter > > Thanks a lot Guenter !! Very useful pointers, will review the > approaches in both the > commits !
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 82b19ebd7838e0..088b6f1fa1ff89 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -4747,7 +4747,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc) mutex_init(&port->lock); mutex_init(&port->swap_lock); - port->wq = create_singlethread_workqueue(dev_name(dev)); + port->wq = alloc_ordered_workqueue("%s", WQ_HIGHPRI, dev_name(dev)); if (!port->wq) return ERR_PTR(-ENOMEM); INIT_DELAYED_WORK(&port->state_machine, tcpm_state_machine_work);
"tReceiverResponse 15 ms Section 6.6.2 The receiver of a Message requiring a response Shall respond within tReceiverResponse in order to ensure that the sender’s SenderResponseTimer does not expire." When the cpu complex is busy running other lower priority work items, TCPM's work queue sometimes does not get scheduled on time to meet the above requirement from the spec. Elevating the TCPM's work queue to higher priority allows TCPM to meet tReceiverResponse in a busy system. Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> --- drivers/usb/typec/tcpm/tcpm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)