* [PATCH] usb: typec: tcpm: Move to high priority workqueue for processing events @ 2020-07-13 20:43 Badhri Jagan Sridharan 2020-07-14 6:05 ` reg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Badhri Jagan Sridharan @ 2020-07-13 20:43 UTC (permalink / raw) To: Guenter Roeck, Heikki Krogerus, reg Kroah-Hartman Cc: linux-usb, linux-kernel, Badhri Jagan Sridharan "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)); if (!port->wq) return ERR_PTR(-ENOMEM); INIT_DELAYED_WORK(&port->state_machine, tcpm_state_machine_work); -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Move to high priority workqueue for processing events 2020-07-13 20:43 [PATCH] usb: typec: tcpm: Move to high priority workqueue for processing events Badhri Jagan Sridharan @ 2020-07-14 6:05 ` reg Kroah-Hartman 2020-07-14 6:57 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: reg Kroah-Hartman @ 2020-07-14 6:05 UTC (permalink / raw) To: Badhri Jagan Sridharan Cc: Guenter Roeck, Heikki Krogerus, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Move to high priority workqueue for processing events 2020-07-14 6:05 ` reg Kroah-Hartman @ 2020-07-14 6:57 ` Guenter Roeck 2020-07-14 17:16 ` Badhri Jagan Sridharan 0 siblings, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2020-07-14 6:57 UTC (permalink / raw) To: reg Kroah-Hartman, Badhri Jagan Sridharan Cc: Heikki Krogerus, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Move to high priority workqueue for processing events 2020-07-14 6:57 ` Guenter Roeck @ 2020-07-14 17:16 ` Badhri Jagan Sridharan 2020-07-23 6:20 ` Badhri Jagan Sridharan 0 siblings, 1 reply; 5+ messages in thread From: Badhri Jagan Sridharan @ 2020-07-14 17:16 UTC (permalink / raw) To: Guenter Roeck; +Cc: reg Kroah-Hartman, Heikki Krogerus, USB, LKML 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 ! ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Move to high priority workqueue for processing events 2020-07-14 17:16 ` Badhri Jagan Sridharan @ 2020-07-23 6:20 ` Badhri Jagan Sridharan 0 siblings, 0 replies; 5+ messages in thread From: Badhri Jagan Sridharan @ 2020-07-23 6:20 UTC (permalink / raw) To: Guenter Roeck; +Cc: reg Kroah-Hartman, Heikki Krogerus, USB, LKML 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 ! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-23 6:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-13 20:43 [PATCH] usb: typec: tcpm: Move to high priority workqueue for processing events Badhri Jagan Sridharan 2020-07-14 6:05 ` reg Kroah-Hartman 2020-07-14 6:57 ` Guenter Roeck 2020-07-14 17:16 ` Badhri Jagan Sridharan 2020-07-23 6:20 ` Badhri Jagan Sridharan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).