"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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
<[email protected]> wrote:
>
> On Mon, Jul 13, 2020 at 11:58 PM Guenter Roeck <[email protected]> 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 <[email protected]>
> > >> ---
> > >> 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 !