[PATCH] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.
Convert the jiffies into usecs then use it with usleep_range such that instead of stuck doing nothing until action happens, sleep with range improves responsiveness and power usage and avoid hacking jiffies. it's used for approximate time. You can check /kernel/time/timer.c. Signed-off-by: Karim Eshapa --- drivers/soc/fsl/qbman/qman.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c index 6f509f6..e0df4d1 100644 --- a/drivers/soc/fsl/qbman/qman.c +++ b/drivers/soc/fsl/qbman/qman.c @@ -1084,11 +1084,9 @@ static int drain_mr_fqrni(struct qm_portal *p) * entries well before the ring has been fully consumed, so * we're being *really* paranoid here. */ - u64 now, then = jiffies; + unsigned int udel_time = jiffies_to_usecs(1); - do { - now = jiffies; - } while ((then + 1) > now); + usleep_range(udel_time/2, udel_time); msg = qm_mr_current(p); if (!msg) return 0; -- 2.7.4
[PATCH] drivers:soc:fsl:qbman:qman.c: unsigned long jiffies value.
unsigned long jiffies value sorry for that. Signed-off-by: Karim Eshapa --- drivers/soc/fsl/qbman/qman.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c index e0df4d1..6e1a44a 100644 --- a/drivers/soc/fsl/qbman/qman.c +++ b/drivers/soc/fsl/qbman/qman.c @@ -1084,7 +1084,7 @@ static int drain_mr_fqrni(struct qm_portal *p) * entries well before the ring has been fully consumed, so * we're being *really* paranoid here. */ - unsigned int udel_time = jiffies_to_usecs(1); + unsigned long udel_time = jiffies_to_usecs(1); usleep_range(udel_time/2, udel_time); msg = qm_mr_current(p); -- 2.7.4
RE:drivers:soc:fsl:qbman:qman.c: unsigned long jiffies value
On Sat, 29 Apr 2017 18:32:55 -0500, Scott Wood wrote: >On Sat, 2017-04-29 at 22:43 +0200, Karim Eshapa wrote: > >> unsigned long jiffies value sorry for that. >> > You mean unsigned long msecs? > Yes, I mean usecs. >> >> Signed-off-by: Karim Eshapa >> --- >> drivers/soc/fsl/qbman/qman.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c >> index e0df4d1..6e1a44a 100644 >> --- a/drivers/soc/fsl/qbman/qman.c >> +++ b/drivers/soc/fsl/qbman/qman.c >> @@ -1084,7 +1084,7 @@ static int drain_mr_fqrni(struct qm_portal *p) >>* entries well before the ring has been fully consumed, so >>* we're being *really* paranoid here. >>*/ >> - unsigned int udel_time = jiffies_to_usecs(1); >> + unsigned long udel_time = jiffies_to_usecs(1); >> >> usleep_range(udel_time/2, udel_time); >> msg = qm_mr_current(p); >If unsigned int isn't big enough, then unsigned long won't be either on 32- >bit. With such a long delay why not use msleep()? > I agree with you in long int. After looking at Documentation/timers/timers-howto.txt I think the msleep() is better as we actually have long delay. may be we can use jiffies_to_msecs() with msleep() in case of we still have this large jiffies difference. >As for the previous patch[1], you're halving the minimum timeout which may not >be correct. > > For the minimum timeout, I've read the following comments. /* * 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); He needs to guarantee certain action so, he made a very large factor of saftey therefore I've used sleep in range with approximate delay. But we still need the driver owner to define appropriate value to put. >[1] When fixing a patch you've already posted that hasn't yet been applied, >send a replacement (v2) patch rather than a separate fix. > Thanks so much, I'm just newbies :) Karim
[PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.
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
RE: [PATCH v3] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.
Use msleep() instead of stucking with long delay will be more efficient. Signed-off-by: Karim Eshapa --- drivers/soc/fsl/qbman/qman.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c index 3d891db..18d391e 100644 --- a/drivers/soc/fsl/qbman/qman.c +++ b/drivers/soc/fsl/qbman/qman.c @@ -1084,11 +1084,7 @@ static int drain_mr_fqrni(struct qm_portal *p) * 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); + msleep(1); msg = qm_mr_current(p); if (!msg) return 0; -- 2.7.4
RE: [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.
>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
[PATCH] drivers:soc:fsl:qbman:qman.c: Change a comment for an entry check inside drain_mr_fqrni function
Change the comment for an entry check inside function drain_mr_fqrni() with sleep for sufficient period of time instead of long time proccessor cycles. Signed-off-by: Karim Eshapa --- drivers/soc/fsl/qbman/qman.c | 25 + 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c index 18d391e..636a7d7 100644 --- a/drivers/soc/fsl/qbman/qman.c +++ b/drivers/soc/fsl/qbman/qman.c @@ -1071,18 +1071,19 @@ static int drain_mr_fqrni(struct qm_portal *p) 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. +* 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 sleep with +* 1 ms would be very efficient, after this period +* we can check if there is something produced. +* 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. */ msleep(1); msg = qm_mr_current(p); -- 2.7.4
[PATCH] drivers:soc:fsl:qbman:qman.c: Change a comment for an entry check inside drain_mr_fqrni function
Change the comment for an entry check inside function drain_mr_fqrni() with sleep for sufficient period of time instead of long time proccessor cycles. Signed-off-by: Karim Eshapa --- drivers/soc/fsl/qbman/qman.c | 25 + 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c index 18d391e..636a7d7 100644 --- a/drivers/soc/fsl/qbman/qman.c +++ b/drivers/soc/fsl/qbman/qman.c @@ -1071,18 +1071,19 @@ static int drain_mr_fqrni(struct qm_portal *p) 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. +* 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 sleep with +* 1 ms would be very efficient, after this period +* we can check if there is something produced. +* 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. */ msleep(1); msg = qm_mr_current(p); -- 2.7.4
[PATCH] soc/qman: Sleep instead of stuck hacking jiffies.
Use msleep() instead of stucking with long delay will be more efficient. Signed-off-by: Karim Eshapa --- drivers/soc/fsl/qbman/qman.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c index 3d891db..18d391e 100644 --- a/drivers/soc/fsl/qbman/qman.c +++ b/drivers/soc/fsl/qbman/qman.c @@ -1084,11 +1084,7 @@ static int drain_mr_fqrni(struct qm_portal *p) * 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); + msleep(1); msg = qm_mr_current(p); if (!msg) return 0; -- 2.7.4
[PATCH] soc/qman: Change a comment for an entry check insid drain_mr_fqrni function
Change the comment for an entry check inside function drain_mr_fqrni() with sleep for sufficient period of time instead of long time proccessor cycles. Signed-off-by: Karim Eshapa --- drivers/soc/fsl/qbman/qman.c | 25 + 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c index 18d391e..636a7d7 100644 --- a/drivers/soc/fsl/qbman/qman.c +++ b/drivers/soc/fsl/qbman/qman.c @@ -1071,18 +1071,19 @@ static int drain_mr_fqrni(struct qm_portal *p) 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. +* 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 sleep with +* 1 ms would be very sufficient, after this period +* we can check if there is something produced. +* 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. */ msleep(1); msg = qm_mr_current(p); -- 2.7.4
Re: drivers:soc:fsl:qbman:qman.c: Change a comment for an entry check inside drain_mr_fqrni function
>Do you mean "sufficient" here rather than "efficient"? It's far less >inefficient than what the code was previously doing, but still... Yes, I'm gonna send a new fix for the comment patch and change the subject of the previous patch soc/qman Thanks, Karim On 25 June 2017 at 04:49, Scott Wood wrote: > On Fri, May 05, 2017 at 10:05:56AM +0200, Karim Eshapa wrote: > > Change the comment for an entry check inside function > > drain_mr_fqrni() with sleep for sufficient period > > of time instead of long time proccessor cycles. > > > > Signed-off-by: Karim Eshapa > > --- > > drivers/soc/fsl/qbman/qman.c | 25 + > > 1 file changed, 13 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c > > index 18d391e..636a7d7 100644 > > --- a/drivers/soc/fsl/qbman/qman.c > > +++ b/drivers/soc/fsl/qbman/qman.c > > @@ -1071,18 +1071,19 @@ static int drain_mr_fqrni(struct qm_portal *p) > > 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. > > + * 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 sleep with > > + * 1 ms would be very efficient, after this period > > + * we can check if there is something produced. > > + * 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. > > Do you mean "sufficient" here rather than "efficient"? It's far less > inefficient than what the code was previously doing, but still... > > Otherwise, looks good. > > -Scott >