Re: [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Scott Wood]

On Fri, 2017-05-05 at 08:01 +0200, Karim Eshapa wrote:
> > On 5/4/2017 5:07 PM, Scott Wood wrote:
> > > On Thu, 2017-05-04 at 06:58 +0200, Karim Eshapa wrote:
> > > > +stop = jiffies + 1;
> > > > +/*
> > > > + * if MR was full and h/w had other FQRNI entries to produce, we
> > > > + * need to allow it time to produce those entries once the
> > > > + * existing entries are consumed. A worst-case situation
> > > > + * (fully-loaded system) means h/w sequencers may have to do 3-4
> > > > + * other things before servicing the portal's MR pump, each of
> > > > + * which (if slow) may take ~50 qman cycles (which is ~200
> > > > + * processor cycles). So rounding up and then multiplying this
> > > > + * worst-case estimate by a factor of 10, just to be
> > > > + * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
> > > > + * one entry at a time, so h/w has an opportunity to produce new
> > > > + * entries well before the ring has been fully consumed, so
> > > > + * we're being *really* paranoid here.
> > > > + */
> > > 
> > > OK, upon reading this more closely it seems the intent was to delay for
> > > 10,000
> > > *processor cycles* and somehow that got turned into 10,000 jiffies
> > > (which is
> > > 40 seconds at the default Hz!).  We could just replace this whole thing
> > > with
> > > msleep(1) and still be far more paranoid than was originally intended.
> > > 
> > > Claudiu and Roy, any comments?
> > 
> > Yes the timing here is certainly off, the code changed a few times since
> > the comment was originally written.
> > An msleep(1) seems reasonable here to me.
> 
> If the previous patch with msleep(1) is OK.
> can I send a patch to slightly change the comments.

Yes.

-Scott

Re: [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Scott Wood]

On Fri, 2017-05-05 at 08:01 +0200, Karim Eshapa wrote:
> > On 5/4/2017 5:07 PM, Scott Wood wrote:
> > > On Thu, 2017-05-04 at 06:58 +0200, Karim Eshapa wrote:
> > > > +stop = jiffies + 1;
> > > > +/*
> > > > + * if MR was full and h/w had other FQRNI entries to produce, we
> > > > + * need to allow it time to produce those entries once the
> > > > + * existing entries are consumed. A worst-case situation
> > > > + * (fully-loaded system) means h/w sequencers may have to do 3-4
> > > > + * other things before servicing the portal's MR pump, each of
> > > > + * which (if slow) may take ~50 qman cycles (which is ~200
> > > > + * processor cycles). So rounding up and then multiplying this
> > > > + * worst-case estimate by a factor of 10, just to be
> > > > + * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
> > > > + * one entry at a time, so h/w has an opportunity to produce new
> > > > + * entries well before the ring has been fully consumed, so
> > > > + * we're being *really* paranoid here.
> > > > + */
> > > 
> > > OK, upon reading this more closely it seems the intent was to delay for
> > > 10,000
> > > *processor cycles* and somehow that got turned into 10,000 jiffies
> > > (which is
> > > 40 seconds at the default Hz!).  We could just replace this whole thing
> > > with
> > > msleep(1) and still be far more paranoid than was originally intended.
> > > 
> > > Claudiu and Roy, any comments?
> > 
> > Yes the timing here is certainly off, the code changed a few times since
> > the comment was originally written.
> > An msleep(1) seems reasonable here to me.
> 
> If the previous patch with msleep(1) is OK.
> can I send a patch to slightly change the comments.

Yes.

-Scott

RE: [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Karim Eshapa]

>On 5/4/2017 5:07 PM, Scott Wood wrote:
>> On Thu, 2017-05-04 at 06:58 +0200, Karim Eshapa wrote:
>>> +stop = jiffies + 1;
>>> +/*
>>> + * if MR was full and h/w had other FQRNI entries to produce, we
>>> + * need to allow it time to produce those entries once the
>>> + * existing entries are consumed. A worst-case situation
>>> + * (fully-loaded system) means h/w sequencers may have to do 3-4
>>> + * other things before servicing the portal's MR pump, each of
>>> + * which (if slow) may take ~50 qman cycles (which is ~200
>>> + * processor cycles). So rounding up and then multiplying this
>>> + * worst-case estimate by a factor of 10, just to be
>>> + * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
>>> + * one entry at a time, so h/w has an opportunity to produce new
>>> + * entries well before the ring has been fully consumed, so
>>> + * we're being *really* paranoid here.
>>> + */
>> OK, upon reading this more closely it seems the intent was to delay for 
>> 10,000
>> *processor cycles* and somehow that got turned into 10,000 jiffies (which is
>> 40 seconds at the default Hz!).  We could just replace this whole thing with
>> msleep(1) and still be far more paranoid than was originally intended.
>>
>> Claudiu and Roy, any comments?
>Yes the timing here is certainly off, the code changed a few times since
>the comment was originally written.
>An msleep(1) seems reasonable here to me.

If the previous patch with msleep(1) is OK.
can I send a patch to slightly change the comments.

Thanks,
Karim

RE: [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Karim Eshapa]

>On 5/4/2017 5:07 PM, Scott Wood wrote:
>> On Thu, 2017-05-04 at 06:58 +0200, Karim Eshapa wrote:
>>> +stop = jiffies + 1;
>>> +/*
>>> + * if MR was full and h/w had other FQRNI entries to produce, we
>>> + * need to allow it time to produce those entries once the
>>> + * existing entries are consumed. A worst-case situation
>>> + * (fully-loaded system) means h/w sequencers may have to do 3-4
>>> + * other things before servicing the portal's MR pump, each of
>>> + * which (if slow) may take ~50 qman cycles (which is ~200
>>> + * processor cycles). So rounding up and then multiplying this
>>> + * worst-case estimate by a factor of 10, just to be
>>> + * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
>>> + * one entry at a time, so h/w has an opportunity to produce new
>>> + * entries well before the ring has been fully consumed, so
>>> + * we're being *really* paranoid here.
>>> + */
>> OK, upon reading this more closely it seems the intent was to delay for 
>> 10,000
>> *processor cycles* and somehow that got turned into 10,000 jiffies (which is
>> 40 seconds at the default Hz!).  We could just replace this whole thing with
>> msleep(1) and still be far more paranoid than was originally intended.
>>
>> Claudiu and Roy, any comments?
>Yes the timing here is certainly off, the code changed a few times since
>the comment was originally written.
>An msleep(1) seems reasonable here to me.

If the previous patch with msleep(1) is OK.
can I send a patch to slightly change the comments.

Thanks,
Karim

Re: [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Roy Pledge]

On 5/4/2017 5:07 PM, Scott Wood wrote:
> On Thu, 2017-05-04 at 06:58 +0200, Karim Eshapa wrote:
>> +stop = jiffies + 1;
>> +/*
>> + * if MR was full and h/w had other FQRNI entries to produce, we
>> + * need to allow it time to produce those entries once the
>> + * existing entries are consumed. A worst-case situation
>> + * (fully-loaded system) means h/w sequencers may have to do 3-4
>> + * other things before servicing the portal's MR pump, each of
>> + * which (if slow) may take ~50 qman cycles (which is ~200
>> + * processor cycles). So rounding up and then multiplying this
>> + * worst-case estimate by a factor of 10, just to be
>> + * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
>> + * one entry at a time, so h/w has an opportunity to produce new
>> + * entries well before the ring has been fully consumed, so
>> + * we're being *really* paranoid here.
>> + */
> OK, upon reading this more closely it seems the intent was to delay for 10,000
> *processor cycles* and somehow that got turned into 10,000 jiffies (which is
> 40 seconds at the default Hz!).  We could just replace this whole thing with
> msleep(1) and still be far more paranoid than was originally intended.
>
> Claudiu and Roy, any comments?
Yes the timing here is certainly off, the code changed a few times since
the comment was originally written.
An msleep(1) seems reasonable here to me.

Roy
>
> -Scott
>
>

Re: [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Roy Pledge]

On 5/4/2017 5:07 PM, Scott Wood wrote:
> On Thu, 2017-05-04 at 06:58 +0200, Karim Eshapa wrote:
>> +stop = jiffies + 1;
>> +/*
>> + * if MR was full and h/w had other FQRNI entries to produce, we
>> + * need to allow it time to produce those entries once the
>> + * existing entries are consumed. A worst-case situation
>> + * (fully-loaded system) means h/w sequencers may have to do 3-4
>> + * other things before servicing the portal's MR pump, each of
>> + * which (if slow) may take ~50 qman cycles (which is ~200
>> + * processor cycles). So rounding up and then multiplying this
>> + * worst-case estimate by a factor of 10, just to be
>> + * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
>> + * one entry at a time, so h/w has an opportunity to produce new
>> + * entries well before the ring has been fully consumed, so
>> + * we're being *really* paranoid here.
>> + */
> OK, upon reading this more closely it seems the intent was to delay for 10,000
> *processor cycles* and somehow that got turned into 10,000 jiffies (which is
> 40 seconds at the default Hz!).  We could just replace this whole thing with
> msleep(1) and still be far more paranoid than was originally intended.
>
> Claudiu and Roy, any comments?
Yes the timing here is certainly off, the code changed a few times since
the comment was originally written.
An msleep(1) seems reasonable here to me.

Roy
>
> -Scott
>
>

Re: [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Scott Wood]

On Thu, 2017-05-04 at 06:58 +0200, Karim Eshapa wrote:
> + stop = jiffies + 1;
> + /*
> +  * if MR was full and h/w had other FQRNI entries to produce, we
> +  * need to allow it time to produce those entries once the
> +  * existing entries are consumed. A worst-case situation
> +  * (fully-loaded system) means h/w sequencers may have to do 3-4
> +  * other things before servicing the portal's MR pump, each of
> +  * which (if slow) may take ~50 qman cycles (which is ~200
> +  * processor cycles). So rounding up and then multiplying this
> +  * worst-case estimate by a factor of 10, just to be
> +  * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
> +  * one entry at a time, so h/w has an opportunity to produce new
> +  * entries well before the ring has been fully consumed, so
> +  * we're being *really* paranoid here.
> +  */

OK, upon reading this more closely it seems the intent was to delay for 10,000
*processor cycles* and somehow that got turned into 10,000 jiffies (which is
40 seconds at the default Hz!).  We could just replace this whole thing with
msleep(1) and still be far more paranoid than was originally intended.

Claudiu and Roy, any comments?

-Scott

Re: [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Scott Wood]

On Thu, 2017-05-04 at 06:58 +0200, Karim Eshapa wrote:
> + stop = jiffies + 1;
> + /*
> +  * if MR was full and h/w had other FQRNI entries to produce, we
> +  * need to allow it time to produce those entries once the
> +  * existing entries are consumed. A worst-case situation
> +  * (fully-loaded system) means h/w sequencers may have to do 3-4
> +  * other things before servicing the portal's MR pump, each of
> +  * which (if slow) may take ~50 qman cycles (which is ~200
> +  * processor cycles). So rounding up and then multiplying this
> +  * worst-case estimate by a factor of 10, just to be
> +  * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
> +  * one entry at a time, so h/w has an opportunity to produce new
> +  * entries well before the ring has been fully consumed, so
> +  * we're being *really* paranoid here.
> +  */

OK, upon reading this more closely it seems the intent was to delay for 10,000
*processor cycles* and somehow that got turned into 10,000 jiffies (which is
40 seconds at the default Hz!).  We could just replace this whole thing with
msleep(1) and still be far more paranoid than was originally intended.

Claudiu and Roy, any comments?

-Scott

[PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Karim Eshapa]

Avoid stuck and hacking jiffies for a long time and using msleep()
for certatin numeber of cylces without the factor of safety
but using the the long delay considering the factor of safety
with the while loop such that after msleep for a short period
of time we check the  msg if it's OK, breaking the big loop delay.

Signed-off-by: Karim Eshapa 
---
 drivers/soc/fsl/qbman/qman.c | 47 ++--
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
index 6f509f6..4f99298 100644
--- a/drivers/soc/fsl/qbman/qman.c
+++ b/drivers/soc/fsl/qbman/qman.c
@@ -1067,32 +1067,33 @@ static irqreturn_t portal_isr(int irq, void *ptr)
 static int drain_mr_fqrni(struct qm_portal *p)
 {
const union qm_mr_entry *msg;
+   unsigned long stop;
+   unsigned int timeout = jiffies_to_msecs(1000);
 loop:
msg = qm_mr_current(p);
-   if (!msg) {
-   /*
-* if MR was full and h/w had other FQRNI entries to produce, we
-* need to allow it time to produce those entries once the
-* existing entries are consumed. A worst-case situation
-* (fully-loaded system) means h/w sequencers may have to do 3-4
-* other things before servicing the portal's MR pump, each of
-* which (if slow) may take ~50 qman cycles (which is ~200
-* processor cycles). So rounding up and then multiplying this
-* worst-case estimate by a factor of 10, just to be
-* ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
-* one entry at a time, so h/w has an opportunity to produce new
-* entries well before the ring has been fully consumed, so
-* we're being *really* paranoid here.
-*/
-   u64 now, then = jiffies;
-
-   do {
-   now = jiffies;
-   } while ((then + 1) > now);
+   stop = jiffies + 1;
+   /*
+* if MR was full and h/w had other FQRNI entries to produce, we
+* need to allow it time to produce those entries once the
+* existing entries are consumed. A worst-case situation
+* (fully-loaded system) means h/w sequencers may have to do 3-4
+* other things before servicing the portal's MR pump, each of
+* which (if slow) may take ~50 qman cycles (which is ~200
+* processor cycles). So rounding up and then multiplying this
+* worst-case estimate by a factor of 10, just to be
+* ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
+* one entry at a time, so h/w has an opportunity to produce new
+* entries well before the ring has been fully consumed, so
+* we're being *really* paranoid here.
+*/
+   do {
+   if (msg)
+   break;
+   msleep(timeout);
msg = qm_mr_current(p);
-   if (!msg)
-   return 0;
-   }
+   } while (time_before(jiffies, stop));
+   if (!msg)
+   return 0;
if ((msg->verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) {
/* We aren't draining anything but FQRNIs */
pr_err("Found verb 0x%x in MR\n", msg->verb);
-- 
2.7.4

[PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.

[Karim Eshapa]

Avoid stuck and hacking jiffies for a long time and using msleep()
for certatin numeber of cylces without the factor of safety
but using the the long delay considering the factor of safety
with the while loop such that after msleep for a short period
of time we check the  msg if it's OK, breaking the big loop delay.

Signed-off-by: Karim Eshapa 
---
 drivers/soc/fsl/qbman/qman.c | 47 ++--
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
index 6f509f6..4f99298 100644
--- a/drivers/soc/fsl/qbman/qman.c
+++ b/drivers/soc/fsl/qbman/qman.c
@@ -1067,32 +1067,33 @@ static irqreturn_t portal_isr(int irq, void *ptr)
 static int drain_mr_fqrni(struct qm_portal *p)
 {
const union qm_mr_entry *msg;
+   unsigned long stop;
+   unsigned int timeout = jiffies_to_msecs(1000);
 loop:
msg = qm_mr_current(p);
-   if (!msg) {
-   /*
-* if MR was full and h/w had other FQRNI entries to produce, we
-* need to allow it time to produce those entries once the
-* existing entries are consumed. A worst-case situation
-* (fully-loaded system) means h/w sequencers may have to do 3-4
-* other things before servicing the portal's MR pump, each of
-* which (if slow) may take ~50 qman cycles (which is ~200
-* processor cycles). So rounding up and then multiplying this
-* worst-case estimate by a factor of 10, just to be
-* ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
-* one entry at a time, so h/w has an opportunity to produce new
-* entries well before the ring has been fully consumed, so
-* we're being *really* paranoid here.
-*/
-   u64 now, then = jiffies;
-
-   do {
-   now = jiffies;
-   } while ((then + 1) > now);
+   stop = jiffies + 1;
+   /*
+* if MR was full and h/w had other FQRNI entries to produce, we
+* need to allow it time to produce those entries once the
+* existing entries are consumed. A worst-case situation
+* (fully-loaded system) means h/w sequencers may have to do 3-4
+* other things before servicing the portal's MR pump, each of
+* which (if slow) may take ~50 qman cycles (which is ~200
+* processor cycles). So rounding up and then multiplying this
+* worst-case estimate by a factor of 10, just to be
+* ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
+* one entry at a time, so h/w has an opportunity to produce new
+* entries well before the ring has been fully consumed, so
+* we're being *really* paranoid here.
+*/
+   do {
+   if (msg)
+   break;
+   msleep(timeout);
msg = qm_mr_current(p);
-   if (!msg)
-   return 0;
-   }
+   } while (time_before(jiffies, stop));
+   if (!msg)
+   return 0;
if ((msg->verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) {
/* We aren't draining anything but FQRNIs */
pr_err("Found verb 0x%x in MR\n", msg->verb);
-- 
2.7.4