[LTP] [PATCH] read_all: retry to queue work for any worker

[LTP] [PATCH] read_all: retry to queue work for any worker

[Jan Stancek]

read_all is currently retrying only for short time period and it's
retrying to queue for same worker. If that worker is busy, it easily
hits timeout.

For example 'kernel_page_tables' on aarch64 can take long time to open/read:
  # time dd if=/sys/kernel/debug/kernel_page_tables of=/dev/null count=1 bs=1024
  1+0 records in
  1+0 records out
  1024 bytes (1.0 kB, 1.0 KiB) copied, 13.0531 s, 0.1 kB/s

  real    0m13.066s
  user    0m0.000s
  sys     0m13.059s

Rather than retrying to queue for specific worker, pick any that can accept
the work and keep trying until we succeed or hit test timeout.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/fs/read_all/read_all.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index 3dac20e02638..4bce4521ace7 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -282,6 +282,33 @@ static void spawn_workers(void)
 	}
 }
 
+static void work_push_retry(int worker, const char *buf)
+{
+	int i, ret, worker_min, worker_max;
+
+	if (worker < 0) {
+		/* pick any, try -worker first */
+		worker_min = worker * (-1);
+		worker_max = worker_count;
+	} else {
+		/* keep trying worker */
+		worker_min = worker;
+		worker_max = worker + 1;
+	}
+	i = worker_min;
+
+	for (;;) {
+		ret = queue_push(workers[i].q, buf);
+		if (ret == 1)
+			break;
+
+		if (++i >= worker_max) {
+			i = worker_min;
+			usleep(100000);
+		}
+	}
+}
+
 static void stop_workers(void)
 {
 	const char stop_code[1] = { '\0' };
@@ -292,7 +319,7 @@ static void stop_workers(void)
 
 	for (i = 0; i < worker_count; i++) {
 		if (workers[i].q)
-			TST_RETRY_FUNC(queue_push(workers[i].q, stop_code), 1);
+			work_push_retry(i, stop_code);
 	}
 
 	for (i = 0; i < worker_count; i++) {
@@ -310,7 +337,7 @@ static void rep_sched_work(const char *path, int rep)
 	for (i = j = 0; i < rep; i++, j++) {
 		if (j >= worker_count)
 			j = 0;
-		TST_RETRY_FUNC(queue_push(workers[j].q, path), 1);
+		work_push_retry(-j, path);
 	}
 }
 
-- 
1.8.3.1

[LTP] [PATCH] read_all: retry to queue work for any worker

[Cyril Hrubis]

Hi!
> +static void work_push_retry(int worker, const char *buf)
> +{
> +	int i, ret, worker_min, worker_max;
> +
> +	if (worker < 0) {
> +		/* pick any, try -worker first */
> +		worker_min = worker * (-1);
> +		worker_max = worker_count;
> +	} else {
> +		/* keep trying worker */
> +		worker_min = worker;
> +		worker_max = worker + 1;
> +	}
> +	i = worker_min;
> +
> +	for (;;) {
> +		ret = queue_push(workers[i].q, buf);
> +		if (ret == 1)
> +			break;
> +
> +		if (++i >= worker_max) {
> +			i = worker_min;
> +			usleep(100000);


My only concern is that we sleep for too long here. Maybe we should
start with smaller sleep here and increase it after a while.

Or have you checked that the tests runs as fast as if we had smaller
sleep here?

> +		}
> +	}
> +}
> +
>  static void stop_workers(void)
>  {
>  	const char stop_code[1] = { '\0' };
> @@ -292,7 +319,7 @@ static void stop_workers(void)
>  
>  	for (i = 0; i < worker_count; i++) {
>  		if (workers[i].q)
> -			TST_RETRY_FUNC(queue_push(workers[i].q, stop_code), 1);
> +			work_push_retry(i, stop_code);
>  	}
>  
>  	for (i = 0; i < worker_count; i++) {
> @@ -310,7 +337,7 @@ static void rep_sched_work(const char *path, int rep)
>  	for (i = j = 0; i < rep; i++, j++) {
>  		if (j >= worker_count)
>  			j = 0;
> -		TST_RETRY_FUNC(queue_push(workers[j].q, path), 1);
> +		work_push_retry(-j, path);
>  	}
>  }
>  
> -- 
> 1.8.3.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] read_all: retry to queue work for any worker

[Jan Stancek]

----- Original Message -----
> Hi!
> > +static void work_push_retry(int worker, const char *buf)
> > +{
> > +	int i, ret, worker_min, worker_max;
> > +
> > +	if (worker < 0) {
> > +		/* pick any, try -worker first */
> > +		worker_min = worker * (-1);
> > +		worker_max = worker_count;
> > +	} else {
> > +		/* keep trying worker */
> > +		worker_min = worker;
> > +		worker_max = worker + 1;
> > +	}
> > +	i = worker_min;
> > +
> > +	for (;;) {
> > +		ret = queue_push(workers[i].q, buf);
> > +		if (ret == 1)
> > +			break;
> > +
> > +		if (++i >= worker_max) {
> > +			i = worker_min;
> > +			usleep(100000);
> 
> 
> My only concern is that we sleep for too long here. Maybe we should
> start with smaller sleep here and increase it after a while.
> 
> Or have you checked that the tests runs as fast as if we had smaller
> sleep here?

Small benchmark shows that smaller sleep can complete faster. I'll send v2.

> 
> > +		}
> > +	}
> > +}
> > +
> >  static void stop_workers(void)
> >  {
> >  	const char stop_code[1] = { '\0' };
> > @@ -292,7 +319,7 @@ static void stop_workers(void)
> >  
> >  	for (i = 0; i < worker_count; i++) {
> >  		if (workers[i].q)
> > -			TST_RETRY_FUNC(queue_push(workers[i].q, stop_code), 1);
> > +			work_push_retry(i, stop_code);
> >  	}
> >  
> >  	for (i = 0; i < worker_count; i++) {
> > @@ -310,7 +337,7 @@ static void rep_sched_work(const char *path, int rep)
> >  	for (i = j = 0; i < rep; i++, j++) {
> >  		if (j >= worker_count)
> >  			j = 0;
> > -		TST_RETRY_FUNC(queue_push(workers[j].q, path), 1);
> > +		work_push_retry(-j, path);
> >  	}
> >  }
> >  
> > --
> > 1.8.3.1
> > 
> > 
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Jan Stancek]

read_all is currently retrying only for short time period and it's
retrying to queue for same worker. If that worker is busy, it easily
hits timeout.

For example 'kernel_page_tables' on aarch64 can take long time to open/read:
  # time dd if=/sys/kernel/debug/kernel_page_tables of=/dev/null count=1 bs=1024
  1+0 records in
  1+0 records out
  1024 bytes (1.0 kB, 1.0 KiB) copied, 13.0531 s, 0.1 kB/s

  real    0m13.066s
  user    0m0.000s
  sys     0m13.059s

Rather than retrying to queue for specific worker, pick any that can accept
the work and keep trying until we succeed or hit test timeout.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/fs/read_all/read_all.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

v2: Increase sleep gradually.

diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index 3dac20e02638..7beb08ccf712 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -282,6 +282,35 @@ static void spawn_workers(void)
 	}
 }
 
+static void work_push_retry(int worker, const char *buf)
+{
+	int i, ret, worker_min, worker_max, usleep_time = 100;
+
+	if (worker < 0) {
+		/* pick any, try -worker first */
+		worker_min = worker * (-1);
+		worker_max = worker_count;
+	} else {
+		/* keep trying worker */
+		worker_min = worker;
+		worker_max = worker + 1;
+	}
+	i = worker_min;
+
+	for (;;) {
+		ret = queue_push(workers[i].q, buf);
+		if (ret == 1)
+			break;
+
+		if (++i >= worker_max) {
+			i = worker_min;
+			if (usleep_time < 100000)
+				usleep_time *= 2;
+			usleep(usleep_time);
+		}
+	}
+}
+
 static void stop_workers(void)
 {
 	const char stop_code[1] = { '\0' };
@@ -292,7 +321,7 @@ static void stop_workers(void)
 
 	for (i = 0; i < worker_count; i++) {
 		if (workers[i].q)
-			TST_RETRY_FUNC(queue_push(workers[i].q, stop_code), 1);
+			work_push_retry(i, stop_code);
 	}
 
 	for (i = 0; i < worker_count; i++) {
@@ -310,7 +339,7 @@ static void rep_sched_work(const char *path, int rep)
 	for (i = j = 0; i < rep; i++, j++) {
 		if (j >= worker_count)
 			j = 0;
-		TST_RETRY_FUNC(queue_push(workers[j].q, path), 1);
+		work_push_retry(-j, path);
 	}
 }
 
-- 
1.8.3.1

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Cyril Hrubis]

Hi!
Looks good, acked.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Li Wang]

On Wed, Oct 9, 2019 at 10:43 PM Jan Stancek <jstancek@redhat.com> wrote:

> read_all is currently retrying only for short time period and it's
> retrying to queue for same worker. If that worker is busy, it easily
> hits timeout.
>
> For example 'kernel_page_tables' on aarch64 can take long time to
> open/read:
>   # time dd if=/sys/kernel/debug/kernel_page_tables of=/dev/null count=1
> bs=1024
>   1+0 records in
>   1+0 records out
>   1024 bytes (1.0 kB, 1.0 KiB) copied, 13.0531 s, 0.1 kB/s
>
>   real    0m13.066s
>   user    0m0.000s
>   sys     0m13.059s
>
> Rather than retrying to queue for specific worker, pick any that can accept
> the work and keep trying until we succeed or hit test timeout.
>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  testcases/kernel/fs/read_all/read_all.c | 33
> +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
>
> v2: Increase sleep gradually.
>
> diff --git a/testcases/kernel/fs/read_all/read_all.c
> b/testcases/kernel/fs/read_all/read_all.c
> index 3dac20e02638..7beb08ccf712 100644
> --- a/testcases/kernel/fs/read_all/read_all.c
> +++ b/testcases/kernel/fs/read_all/read_all.c
> @@ -282,6 +282,35 @@ static void spawn_workers(void)
>         }
>  }
>
> +static void work_push_retry(int worker, const char *buf)
> +{
> +       int i, ret, worker_min, worker_max, usleep_time = 100;
> +
> +       if (worker < 0) {
> +               /* pick any, try -worker first */
> +               worker_min = worker * (-1);
> +               worker_max = worker_count;
> +       } else {
> +               /* keep trying worker */
> +               worker_min = worker;
> +               worker_max = worker + 1;
> +       }
> +       i = worker_min;
> +
> +       for (;;) {
> +               ret = queue_push(workers[i].q, buf);
> +               if (ret == 1)
> +                       break;
> +
> +               if (++i >= worker_max) {
> +                       i = worker_min;
> +                       if (usleep_time < 100000)
> +                               usleep_time *= 2;
> +                       usleep(usleep_time);
>

At first, I thought of TST_RETRY_FN_EXP_BACKOFF, but it seems not easy to
find the proper value of MAX_DELAY, so you method looks more stable to
read_all.

   Acked-by: Li Wang <liwang@redhat.com>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191011/aae2d179/attachment.htm>

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Li Wang]

Hi Jan,

On Fri, Oct 11, 2019 at 4:24 PM Li Wang <liwang@redhat.com> wrote:

>
>
> On Wed, Oct 9, 2019 at 10:43 PM Jan Stancek <jstancek@redhat.com> wrote:
>
>> read_all is currently retrying only for short time period and it's
>> retrying to queue for same worker. If that worker is busy, it easily
>> hits timeout.
>>
>> For example 'kernel_page_tables' on aarch64 can take long time to
>> open/read:
>>   # time dd if=/sys/kernel/debug/kernel_page_tables of=/dev/null count=1
>> bs=1024
>>   1+0 records in
>>   1+0 records out
>>   1024 bytes (1.0 kB, 1.0 KiB) copied, 13.0531 s, 0.1 kB/s
>>
>>   real    0m13.066s
>>   user    0m0.000s
>>   sys     0m13.059s
>>
>> Rather than retrying to queue for specific worker, pick any that can
>> accept
>> the work and keep trying until we succeed or hit test timeout.
>>
>
RFC:

Base on your patch, I'm thinking to achieve a new macro TST_INFILOOP_FUNC
which can repeat the @FUNC infinitely. Do you feel it satisfies your
requirements to some degree or meaningful to LTP?
+/**
+ * TST_INFILOOP_FUNC() - Infinitely retry a function with an increasing
delay.
+ * @FUNC - The function which will be retried
+ * @ERET - The value returned from @FUNC on success
+ *
+ * This macro will call @FUNC in an infinite loop with a delay. If @FUNC
+ * returns @ERET then the loop exits. The delay between retries starts at
one
+ * microsecond and is then doubled each iteration until it exceeds one
second.
+ * When the delay exceeds one-second @FUNC keep repeat until get success
or hit
+ * test timeout.
+ */
+#define TST_INFILOOP_FUNC(FUNC, ERET) \
+       TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, -1)
+
 #define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY)        \
-({     int tst_delay_ = 1;                                             \
+({     int tst_delay_ = 1, tst_max_delay_ = MAX_DELAY;                 \
+       if (MAX_DELAY < 0)                                              \
+                tst_max_delay_ *= MAX_DELAY;                           \
        for (;;) {                                                      \
                typeof(FUNC) tst_ret_ = FUNC;                           \
                if (tst_ret_ == ERET)                                   \
                        break;                                          \
-               if (tst_delay_ < MAX_DELAY * 1000000) {                 \
-                       usleep(tst_delay_);                             \
+               usleep(tst_delay_);                                     \
+               if (tst_delay_ < tst_max_delay_ * 1000000) {            \
                        tst_delay_ *= 2;                                \
                } else {                                                \
-                       tst_brk(TBROK, #FUNC" timed out");              \
+                        if (MAX_DELAY > 0)                             \
+                               tst_brk(TBROK, #FUNC" timed out");      \
                }                                                       \
        }                                                               \
        ERET;                                                           \

Add pastebin to better readable: http://pastebin.test.redhat.com/805437

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191012/782621dd/attachment-0001.htm>

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Jan Stancek]

----- Original Message -----
> Hi Jan,
> 
> On Fri, Oct 11, 2019 at 4:24 PM Li Wang <liwang@redhat.com> wrote:
> 
> >
> >
> > On Wed, Oct 9, 2019 at 10:43 PM Jan Stancek <jstancek@redhat.com> wrote:
> >
> >> read_all is currently retrying only for short time period and it's
> >> retrying to queue for same worker. If that worker is busy, it easily
> >> hits timeout.
> >>
> >> For example 'kernel_page_tables' on aarch64 can take long time to
> >> open/read:
> >>   # time dd if=/sys/kernel/debug/kernel_page_tables of=/dev/null count=1
> >> bs=1024
> >>   1+0 records in
> >>   1+0 records out
> >>   1024 bytes (1.0 kB, 1.0 KiB) copied, 13.0531 s, 0.1 kB/s
> >>
> >>   real    0m13.066s
> >>   user    0m0.000s
> >>   sys     0m13.059s
> >>
> >> Rather than retrying to queue for specific worker, pick any that can
> >> accept
> >> the work and keep trying until we succeed or hit test timeout.
> >>
> >
> RFC:
> 
> Base on your patch, I'm thinking to achieve a new macro TST_INFILOOP_FUNC
> which can repeat the @FUNC infinitely. Do you feel it satisfies your
> requirements to some degree or meaningful to LTP?

I'm OK with concept. I'd like more some variation of *RETRY* for name.
Comments below.

> +/**
> + * TST_INFILOOP_FUNC() - Infinitely retry a function with an increasing
> delay.
> + * @FUNC - The function which will be retried
> + * @ERET - The value returned from @FUNC on success
> + *
> + * This macro will call @FUNC in an infinite loop with a delay. If @FUNC
> + * returns @ERET then the loop exits. The delay between retries starts at
> one
> + * microsecond and is then doubled each iteration until it exceeds one
> second.
> + * When the delay exceeds one-second @FUNC keep repeat until get success
> or hit
> + * test timeout.
> + */
> +#define TST_INFILOOP_FUNC(FUNC, ERET) \
> +       TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, -1)
> +
>  #define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY)        \
> -({     int tst_delay_ = 1;                                             \
> +({     int tst_delay_ = 1, tst_max_delay_ = MAX_DELAY;                 \
> +       if (MAX_DELAY < 0)                                              \
> +                tst_max_delay_ *= MAX_DELAY;                           \

Shouldn't this be just times (-1). For -5 you get 25 as max sleep time.

>         for (;;) {                                                      \
>                 typeof(FUNC) tst_ret_ = FUNC;                           \
>                 if (tst_ret_ == ERET)                                   \
>                         break;                                          \
> -               if (tst_delay_ < MAX_DELAY * 1000000) {                 \
> -                       usleep(tst_delay_);                             \
> +               usleep(tst_delay_);                                     \
> +               if (tst_delay_ < tst_max_delay_ * 1000000) {            \
>                         tst_delay_ *= 2;                                \
>                 } else {                                                \
> -                       tst_brk(TBROK, #FUNC" timed out");              \
> +                        if (MAX_DELAY > 0)                             \

pastebin has this condition backwards, but here it looks ok.

> +                               tst_brk(TBROK, #FUNC" timed out");      \
>                 }                                                       \
>         }                                                               \
>         ERET;                                                           \
> 
> Add pastebin to better readable: http://pastebin.test.redhat.com/805437
> 
> --
> Regards,
> Li Wang
> 

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Li Wang]

On Sat, Oct 12, 2019 at 2:17 PM Jan Stancek <jstancek@redhat.com> wrote:

>
>
> ----- Original Message -----
> > Hi Jan,
> >
> > On Fri, Oct 11, 2019 at 4:24 PM Li Wang <liwang@redhat.com> wrote:
> >
> > >
> > >
> > > On Wed, Oct 9, 2019 at 10:43 PM Jan Stancek <jstancek@redhat.com>
> wrote:
> > >
> > >> read_all is currently retrying only for short time period and it's
> > >> retrying to queue for same worker. If that worker is busy, it easily
> > >> hits timeout.
> > >>
> > >> For example 'kernel_page_tables' on aarch64 can take long time to
> > >> open/read:
> > >>   # time dd if=/sys/kernel/debug/kernel_page_tables of=/dev/null
> count=1
> > >> bs=1024
> > >>   1+0 records in
> > >>   1+0 records out
> > >>   1024 bytes (1.0 kB, 1.0 KiB) copied, 13.0531 s, 0.1 kB/s
> > >>
> > >>   real    0m13.066s
> > >>   user    0m0.000s
> > >>   sys     0m13.059s
> > >>
> > >> Rather than retrying to queue for specific worker, pick any that can
> > >> accept
> > >> the work and keep trying until we succeed or hit test timeout.
> > >>
> > >
> > RFC:
> >
> > Base on your patch, I'm thinking to achieve a new macro TST_INFILOOP_FUNC
> > which can repeat the @FUNC infinitely. Do you feel it satisfies your
> > requirements to some degree or meaningful to LTP?
>
> I'm OK with concept. I'd like more some variation of *RETRY* for name.
> Comments below.
>

Thanks, what about naming: TST_INFI_RETRY_FUNC?

And do you mind use it to replace your function work_push_retry()? I know
it may be not smarter than work_push_retry() but it looks tiny for code.

> ...
> > +#define TST_INFILOOP_FUNC(FUNC, ERET) \
> > +       TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, -1)
> > +
> >  #define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY)        \
> > -({     int tst_delay_ = 1;                                             \
> > +({     int tst_delay_ = 1, tst_max_delay_ = MAX_DELAY;                 \
> > +       if (MAX_DELAY < 0)                                              \
> > +                tst_max_delay_ *= MAX_DELAY;                           \
>
> Shouldn't this be just times (-1). For -5 you get 25 as max sleep time.
>

Agree.

>
> >         for (;;) {                                                      \
> >                 typeof(FUNC) tst_ret_ = FUNC;                           \
> >                 if (tst_ret_ == ERET)                                   \
> >                         break;                                          \
> > -               if (tst_delay_ < MAX_DELAY * 1000000) {                 \
> > -                       usleep(tst_delay_);                             \
> > +               usleep(tst_delay_);                                     \
> > +               if (tst_delay_ < tst_max_delay_ * 1000000) {            \
> >                         tst_delay_ *= 2;                                \
> >                 } else {                                                \
> > -                       tst_brk(TBROK, #FUNC" timed out");              \
> > +                        if (MAX_DELAY > 0)                             \
>
> pastebin has this condition backwards, but here it looks ok

Sorry for the typo in pastebin.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191012/beccb1b9/attachment.htm>

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Jan Stancek]

----- Original Message -----
> On Sat, Oct 12, 2019 at 2:17 PM Jan Stancek <jstancek@redhat.com> wrote:
> 
> >
> >
> > ----- Original Message -----
> > > Hi Jan,
> > >
> > > On Fri, Oct 11, 2019 at 4:24 PM Li Wang <liwang@redhat.com> wrote:
> > >
> > > >
> > > >
> > > > On Wed, Oct 9, 2019 at 10:43 PM Jan Stancek <jstancek@redhat.com>
> > wrote:
> > > >
> > > >> read_all is currently retrying only for short time period and it's
> > > >> retrying to queue for same worker. If that worker is busy, it easily
> > > >> hits timeout.
> > > >>
> > > >> For example 'kernel_page_tables' on aarch64 can take long time to
> > > >> open/read:
> > > >>   # time dd if=/sys/kernel/debug/kernel_page_tables of=/dev/null
> > count=1
> > > >> bs=1024
> > > >>   1+0 records in
> > > >>   1+0 records out
> > > >>   1024 bytes (1.0 kB, 1.0 KiB) copied, 13.0531 s, 0.1 kB/s
> > > >>
> > > >>   real    0m13.066s
> > > >>   user    0m0.000s
> > > >>   sys     0m13.059s
> > > >>
> > > >> Rather than retrying to queue for specific worker, pick any that can
> > > >> accept
> > > >> the work and keep trying until we succeed or hit test timeout.
> > > >>
> > > >
> > > RFC:
> > >
> > > Base on your patch, I'm thinking to achieve a new macro TST_INFILOOP_FUNC
> > > which can repeat the @FUNC infinitely. Do you feel it satisfies your
> > > requirements to some degree or meaningful to LTP?
> >
> > I'm OK with concept. I'd like more some variation of *RETRY* for name.
> > Comments below.
> >
> 
> Thanks, what about naming: TST_INFI_RETRY_FUNC?

Or just keep TST_RETRY_FUNC and add parameter to it?

> 
> And do you mind use it to replace your function work_push_retry()? I know
> it may be not smarter than work_push_retry() but it looks tiny for code.

It may need some wrapper, because work_push_retry() may be passing different
argument to function on each retry, which was one of reasons for the patch.

> 
> > ...
> > > +#define TST_INFILOOP_FUNC(FUNC, ERET) \
> > > +       TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, -1)
> > > +
> > >  #define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY)        \
> > > -({     int tst_delay_ = 1;                                             \
> > > +({     int tst_delay_ = 1, tst_max_delay_ = MAX_DELAY;                 \
> > > +       if (MAX_DELAY < 0)                                              \
> > > +                tst_max_delay_ *= MAX_DELAY;                           \
> >
> > Shouldn't this be just times (-1). For -5 you get 25 as max sleep time.
> >
> 
> Agree.
> 
> >
> > >         for (;;) {                                                      \
> > >                 typeof(FUNC) tst_ret_ = FUNC;                           \
> > >                 if (tst_ret_ == ERET)                                   \
> > >                         break;                                          \
> > > -               if (tst_delay_ < MAX_DELAY * 1000000) {                 \
> > > -                       usleep(tst_delay_);                             \
> > > +               usleep(tst_delay_);                                     \
> > > +               if (tst_delay_ < tst_max_delay_ * 1000000) {            \
> > >                         tst_delay_ *= 2;                                \
> > >                 } else {                                                \
> > > -                       tst_brk(TBROK, #FUNC" timed out");              \
> > > +                        if (MAX_DELAY > 0)                             \
> >
> > pastebin has this condition backwards, but here it looks ok
> 
> Sorry for the typo in pastebin.
> 
> --
> Regards,
> Li Wang
> 

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Li Wang]

On Sat, Oct 12, 2019 at 2:49 PM Jan Stancek <jstancek@redhat.com> wrote:

> ...
> > > > Base on your patch, I'm thinking to achieve a new macro
> TST_INFILOOP_FUNC
> > > > which can repeat the @FUNC infinitely. Do you feel it satisfies your
> > > > requirements to some degree or meaningful to LTP?
> > >
> > > I'm OK with concept. I'd like more some variation of *RETRY* for name.
> > > Comments below.
> > >
> >
> > Thanks, what about naming: TST_INFI_RETRY_FUNC?
>
> Or just keep TST_RETRY_FUNC and add parameter to it?
>

Sounds better, we could add parameter @INFI to control the retry as a knob.

/* @INFI - 1: retry infinitely, 0: retry in limit times */

#define TST_RETRY_FUNC(FUNC, ERET, INFI) \
        TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, 1, INFI)


> >
> > And do you mind use it to replace your function work_push_retry()? I know
> > it may be not smarter than work_push_retry() but it looks tiny for code.
>
> It may need some wrapper, because work_push_retry() may be passing
> different
> argument to function on each retry, which was one of reasons for the patch.
>

I was not meaning to hack the work_push_retry() function, I mean to change
your patch as below after we improve the TST_RETRY_FUNC.

# git diff .
diff --git a/testcases/kernel/fs/read_all/read_all.c
b/testcases/kernel/fs/read_all/read_all.c
index 3dac20e..ccbc5eb 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -292,7 +292,7 @@ static void stop_workers(void)

        for (i = 0; i < worker_count; i++) {
                if (workers[i].q)
-                       TST_RETRY_FUNC(queue_push(workers[i].q, stop_code),
1);
+                       TST_RETRY_FUNC(queue_push(workers[i].q, stop_code),
1, 1);
        }

        for (i = 0; i < worker_count; i++) {
@@ -310,7 +310,7 @@ static void rep_sched_work(const char *path, int rep)
        for (i = j = 0; i < rep; i++, j++) {
                if (j >= worker_count)
                        j = 0;
-               TST_RETRY_FUNC(queue_push(workers[j].q, path), 1);
+               TST_RETRY_FUNC(queue_push(workers[j].q, path), 1, 1);
        }
 }


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191012/10131c99/attachment-0001.htm>

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Jan Stancek]

----- Original Message -----
> On Sat, Oct 12, 2019 at 2:49 PM Jan Stancek <jstancek@redhat.com> wrote:
> 
> > ...
> > > > > Base on your patch, I'm thinking to achieve a new macro
> > TST_INFILOOP_FUNC
> > > > > which can repeat the @FUNC infinitely. Do you feel it satisfies your
> > > > > requirements to some degree or meaningful to LTP?
> > > >
> > > > I'm OK with concept. I'd like more some variation of *RETRY* for name.
> > > > Comments below.
> > > >
> > >
> > > Thanks, what about naming: TST_INFI_RETRY_FUNC?
> >
> > Or just keep TST_RETRY_FUNC and add parameter to it?
> >
> 
> Sounds better, we could add parameter @INFI to control the retry as a knob.
> 
> /* @INFI - 1: retry infinitely, 0: retry in limit times */
> 
> #define TST_RETRY_FUNC(FUNC, ERET, INFI) \
>         TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, 1, INFI)

Other option is we use directly TST_RETRY_FN_EXP_BACKOFF.

> 
> 
> > >
> > > And do you mind use it to replace your function work_push_retry()? I know
> > > it may be not smarter than work_push_retry() but it looks tiny for code.
> >
> > It may need some wrapper, because work_push_retry() may be passing
> > different
> > argument to function on each retry, which was one of reasons for the patch.
> >
> 
> I was not meaning to hack the work_push_retry() function, I mean to change
> your patch as below after we improve the TST_RETRY_FUNC.

Why not? Wouldn't we get better performance if we don't wait on specific worker
to complete?

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Li Wang]

On Sun, Oct 13, 2019 at 3:54 PM Jan Stancek <jstancek@redhat.com> wrote:

>
>
> ----- Original Message -----
> > On Sat, Oct 12, 2019 at 2:49 PM Jan Stancek <jstancek@redhat.com> wrote:
> >
> > > ...
> > > > > > Base on your patch, I'm thinking to achieve a new macro
> > > TST_INFILOOP_FUNC
> > > > > > which can repeat the @FUNC infinitely. Do you feel it satisfies
> your
> > > > > > requirements to some degree or meaningful to LTP?
> > > > >
> > > > > I'm OK with concept. I'd like more some variation of *RETRY* for
> name.
> > > > > Comments below.
> > > > >
> > > >
> > > > Thanks, what about naming: TST_INFI_RETRY_FUNC?
> > >
> > > Or just keep TST_RETRY_FUNC and add parameter to it?
> > >
> >
> > Sounds better, we could add parameter @INFI to control the retry as a
> knob.
> >
> > /* @INFI - 1: retry infinitely, 0: retry in limit times */
> >
> > #define TST_RETRY_FUNC(FUNC, ERET, INFI) \
> >         TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, 1, INFI)
>
> Other option is we use directly TST_RETRY_FN_EXP_BACKOFF.
>

I'm ok for either. I will have a try when I get a chance since mkswap01.sh
sometimes timed out in the one-second retrying.


>
> >
> >
> > > >
> > > > And do you mind use it to replace your function work_push_retry()? I
> know
> > > > it may be not smarter than work_push_retry() but it looks tiny for
> code.
> > >
> > > It may need some wrapper, because work_push_retry() may be passing
> > > different
> > > argument to function on each retry, which was one of reasons for the
> patch.
> > >
> >
> > I was not meaning to hack the work_push_retry() function, I mean to
> change
> > your patch as below after we improve the TST_RETRY_FUNC.
>
> Why not? Wouldn't we get better performance if we don't wait on specific
> worker
> to complete?
>
Well, right! You can merge this patch as it is.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191014/37921d27/attachment.htm>

[LTP] [PATCH v2] read_all: retry to queue work for any worker

[Jan Stancek]

----- Original Message -----
> Well, right! You can merge this patch as it is.

Pushed.