* [LTP] [PATCH 1/1] hugeshmctl01: Fix reset stat_time when looping with -i n
@ 2020-03-05 17:18 Petr Vorel
2020-03-06 1:53 ` Yang Xu
0 siblings, 1 reply; 5+ messages in thread
From: Petr Vorel @ 2020-03-05 17:18 UTC (permalink / raw)
To: ltp
c7a2d296b didn't reset stat_time, thus uninitialized set_shared was
assigned to test variable and test got value from a null pointer,
which leaded to segfault.
hugeshmctl01.c:279: PASS: shmctl in func_rmid() failed as expected, shared memory appears to be removed
tst_checkpoint.c:147: BROK: hugeshmctl01.c:152: tst_checkpoint_wait(0, 10000): ETIMEDOUT (110)
mem.c:817: INFO: set nr_hugepages to 0
dmesg:
segfault at 7f73f8c00000 ip 00000000004051e1 sp 00007ffef375f9a0 error 6 in hugeshmctl01.master[404000+13000]
addr2line -e hugeshmctl01 -f 00000000004051e1
stat_setup
hugeshmctl01.c:139 (discriminator 4)
test = (stat_time == FIRST) ? set_shmat() : set_shared;
/* do an assignement for fun */
*(int *)test = i; // error here
Fixes: c7a2d296b ("hugeshmctl: Use loop from the API")
Reported-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
Suggested-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Hi Xu,
I'm sorry for introducing a regression.
Thank you for a report and fixing the test.
I'd personally prefer to keep .tcnt = ARRAY_SIZE(tcases),
but maybe others will prefer to keep loop in test_hugeshmctl()
as it was before my change.
BTW it'd be nice to have something like setup_i() and cleanup_i(),
which would be called before/after each run of whole test, when run with
-i n.
Kind regards,
Petr
.../mem/hugetlb/hugeshmctl/hugeshmctl01.c | 27 ++++++++++---------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
index e6cf8bf09..3b7e14363 100644
--- a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
+++ b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
@@ -75,6 +75,20 @@ struct tcase {
static void test_hugeshmctl(unsigned int i)
{
+ stat_time = FIRST;
+
+ /*
+ * Create a shared memory segment with read and write
+ * permissions. Do this here instead of in setup()
+ * so that looping (-i) will work correctly.
+ */
+ if (i == 0) {
+ shm_id_1 = shmget(shmkey, shm_size,
+ SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
+ if (shm_id_1 == -1)
+ tst_brk(TBROK | TERRNO, "shmget #main");
+ }
+
/*
* if needed, set up any required conditions by
* calling the appropriate setup function
@@ -296,19 +310,6 @@ void setup(void)
shm_size = hpage_size * hugepages / 2;
update_shm_size(&shm_size);
shmkey = getipckey();
-
- /* initialize stat_time */
- stat_time = FIRST;
-
- /*
- * Create a shared memory segment with read and write
- * permissions. Do this here instead of in setup()
- * so that looping (-i) will work correctly.
- */
- shm_id_1 = shmget(shmkey, shm_size,
- SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
- if (shm_id_1 == -1)
- tst_brk(TBROK | TERRNO, "shmget #main");
}
void cleanup(void)
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [LTP] [PATCH 1/1] hugeshmctl01: Fix reset stat_time when looping with -i n
2020-03-05 17:18 [LTP] [PATCH 1/1] hugeshmctl01: Fix reset stat_time when looping with -i n Petr Vorel
@ 2020-03-06 1:53 ` Yang Xu
2020-03-06 3:29 ` Xiao Yang
2020-03-06 5:43 ` Li Wang
0 siblings, 2 replies; 5+ messages in thread
From: Yang Xu @ 2020-03-06 1:53 UTC (permalink / raw)
To: ltp
Hi Petr
> c7a2d296b didn't reset stat_time, thus uninitialized set_shared was
> assigned to test variable and test got value from a null pointer,
> which leaded to segfault.
>
> hugeshmctl01.c:279: PASS: shmctl in func_rmid() failed as expected, shared memory appears to be removed
> tst_checkpoint.c:147: BROK: hugeshmctl01.c:152: tst_checkpoint_wait(0, 10000): ETIMEDOUT (110)
> mem.c:817: INFO: set nr_hugepages to 0
>
> dmesg:
> segfault at 7f73f8c00000 ip 00000000004051e1 sp 00007ffef375f9a0 error 6 in hugeshmctl01.master[404000+13000]
> addr2line -e hugeshmctl01 -f 00000000004051e1
> stat_setup
> hugeshmctl01.c:139 (discriminator 4)
>
> test = (stat_time == FIRST) ? set_shmat() : set_shared;
>
> /* do an assignement for fun */
> *(int *)test = i; // error here
>
> Fixes: c7a2d296b ("hugeshmctl: Use loop from the API")
>
> Reported-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> Suggested-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Hi Xu,
>
> I'm sorry for introducing a regression.
> Thank you for a report and fixing the test.
> I'd personally prefer to keep .tcnt = ARRAY_SIZE(tcases),
> but maybe others will prefer to keep loop in test_hugeshmctl()
> as it was before my change.
>
> BTW it'd be nice to have something like setup_i() and cleanup_i(),
> which would be called before/after each run of whole test, when run with
> -i n.
Sub tests have own clean and setup function. They only reused a System
V shared memory segment. IMO, this is a question of coupling.
>
> Kind regards,
> Petr
>
> .../mem/hugetlb/hugeshmctl/hugeshmctl01.c | 27 ++++++++++---------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> index e6cf8bf09..3b7e14363 100644
> --- a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> +++ b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> @@ -75,6 +75,20 @@ struct tcase {
>
> static void test_hugeshmctl(unsigned int i)
> {
> + stat_time = FIRST;
> +
My description may confuse you. stat_time should not be reseted every
time, it only needs to be reseted when next loop. This value will be +1
when call stat_cleanup.
struct tcase {
int cmd;
void (*func_test) (void);
void (*func_setup) (void);
} tcases[] = {
{IPC_STAT, func_stat, stat_setup}, //stat_time = FIRST
{IPC_STAT, func_stat, stat_setup}, //stat_time = SECOND
As you do, the first and second case are same. it should be added into
the "if == 0".
ps: I personally think old case is more cleaner. Let's hear from others.
Best Regards
Yang Xu
> + /*
> + * Create a shared memory segment with read and write
> + * permissions. Do this here instead of in setup()
> + * so that looping (-i) will work correctly.
> + */
> + if (i == 0) {
> + shm_id_1 = shmget(shmkey, shm_size,
> + SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
> + if (shm_id_1 == -1)
> + tst_brk(TBROK | TERRNO, "shmget #main");
> + }
> +
> /*
> * if needed, set up any required conditions by
> * calling the appropriate setup function
> @@ -296,19 +310,6 @@ void setup(void)
> shm_size = hpage_size * hugepages / 2;
> update_shm_size(&shm_size);
> shmkey = getipckey();
> -
> - /* initialize stat_time */
> - stat_time = FIRST;
> -
> - /*
> - * Create a shared memory segment with read and write
> - * permissions. Do this here instead of in setup()
> - * so that looping (-i) will work correctly.
> - */
> - shm_id_1 = shmget(shmkey, shm_size,
> - SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
> - if (shm_id_1 == -1)
> - tst_brk(TBROK | TERRNO, "shmget #main");
> }
>
> void cleanup(void)
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [LTP] [PATCH 1/1] hugeshmctl01: Fix reset stat_time when looping with -i n
2020-03-06 1:53 ` Yang Xu
@ 2020-03-06 3:29 ` Xiao Yang
2020-03-06 5:43 ` Li Wang
1 sibling, 0 replies; 5+ messages in thread
From: Xiao Yang @ 2020-03-06 3:29 UTC (permalink / raw)
To: ltp
On 2020/3/6 9:53, Yang Xu wrote:
>> static void test_hugeshmctl(unsigned int i)
>> {
>> + stat_time = FIRST;
>> +
> My description may confuse you. stat_time should not be reseted every
> time, it only needs to be reseted when next loop. This value will be +1
> when call stat_cleanup.
> struct tcase {
> int cmd;
> void (*func_test) (void);
> void (*func_setup) (void);
> } tcases[] = {
> {IPC_STAT, func_stat, stat_setup}, //stat_time = FIRST
> {IPC_STAT, func_stat, stat_setup}, //stat_time = SECOND
>
> As you do, the first and second case are same. it should be added into
> the "if == 0".
>
> ps: I personally think old case is more cleaner. Let's hear from others.
>
Hi Petr, Xu
For xu's comment, we can assign 'i' to stat_time and remove
"stat_time++;" directly:
----------------------------------------
static void test_hugeshmctl(unsigned int i)
{
+ stat_time = i;
+
...
- stat_time++;
----------------------------------------
Thanks,
Xiao Yang
^ permalink raw reply [flat|nested] 5+ messages in thread
* [LTP] [PATCH 1/1] hugeshmctl01: Fix reset stat_time when looping with -i n
2020-03-06 1:53 ` Yang Xu
2020-03-06 3:29 ` Xiao Yang
@ 2020-03-06 5:43 ` Li Wang
2020-03-06 6:12 ` Yang Xu
1 sibling, 1 reply; 5+ messages in thread
From: Li Wang @ 2020-03-06 5:43 UTC (permalink / raw)
To: ltp
On Fri, Mar 6, 2020 at 9:53 AM Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote:
> Hi Petr
>
> > c7a2d296b didn't reset stat_time, thus uninitialized set_shared was
> > assigned to test variable and test got value from a null pointer,
> > which leaded to segfault.
> >
> > hugeshmctl01.c:279: PASS: shmctl in func_rmid() failed as expected,
> shared memory appears to be removed
> > tst_checkpoint.c:147: BROK: hugeshmctl01.c:152: tst_checkpoint_wait(0,
> 10000): ETIMEDOUT (110)
> > mem.c:817: INFO: set nr_hugepages to 0
> >
> > dmesg:
> > segfault at 7f73f8c00000 ip 00000000004051e1 sp 00007ffef375f9a0 error 6
> in hugeshmctl01.master[404000+13000]
> > addr2line -e hugeshmctl01 -f 00000000004051e1
> > stat_setup
> > hugeshmctl01.c:139 (discriminator 4)
> >
> > test = (stat_time == FIRST) ? set_shmat() : set_shared;
> >
> > /* do an assignement for fun */
> > *(int *)test = i; // error here
> >
> > Fixes: c7a2d296b ("hugeshmctl: Use loop from the API")
> >
> > Reported-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> > Suggested-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > Hi Xu,
> >
> > I'm sorry for introducing a regression.
> > Thank you for a report and fixing the test.
> > I'd personally prefer to keep .tcnt = ARRAY_SIZE(tcases),
> > but maybe others will prefer to keep loop in test_hugeshmctl()
> > as it was before my change.
> >
> > BTW it'd be nice to have something like setup_i() and cleanup_i(),
> > which would be called before/after each run of whole test, when run with
> > -i n.
> Sub tests have own clean and setup function. They only reused a System
> V shared memory segment. IMO, this is a question of coupling.
> >
> > Kind regards,
> > Petr
> >
> > .../mem/hugetlb/hugeshmctl/hugeshmctl01.c | 27 ++++++++++---------
> > 1 file changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> > index e6cf8bf09..3b7e14363 100644
> > --- a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> > +++ b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> > @@ -75,6 +75,20 @@ struct tcase {
> >
> > static void test_hugeshmctl(unsigned int i)
> > {
> > + stat_time = FIRST;
> > +
> My description may confuse you. stat_time should not be reseted every
> time, it only needs to be reseted when next loop. This value will be +1
> when call stat_cleanup.
>
Xu Yang is correct here. If we do reset 'stat_time' to FIRST in each loop
then it would be missing the SECOND part in the stat_setup(). We can fix
the problem simply to move the 'stat_time' to if-condition as he suggested.
But to be honest, the looping workflow of hugeshmctl01 looks a little bit
in disorder that may be the reason makes Petr missed the 'stat_time' value
part, I appreciated if someone who can help to refactor the hugeshmctl01:).
> struct tcase {
> int cmd;
> void (*func_test) (void);
> void (*func_setup) (void);
> } tcases[] = {
> {IPC_STAT, func_stat, stat_setup}, //stat_time = FIRST
> {IPC_STAT, func_stat, stat_setup}, //stat_time = SECOND
>
> As you do, the first and second case are same. it should be added into
> the "if == 0".
>
> ps: I personally think old case is more cleaner. Let's hear from others.
>
> Best Regards
> Yang Xu
> > + /*
> > + * Create a shared memory segment with read and write
> > + * permissions. Do this here instead of in setup()
> > + * so that looping (-i) will work correctly.
> > + */
> > + if (i == 0) {
> > + shm_id_1 = shmget(shmkey, shm_size,
> > + SHM_HUGETLB | IPC_CREAT | IPC_EXCL |
> SHM_RW);
> > + if (shm_id_1 == -1)
> > + tst_brk(TBROK | TERRNO, "shmget #main");
> > + }
> > +
> > /*
> > * if needed, set up any required conditions by
> > * calling the appropriate setup function
> > @@ -296,19 +310,6 @@ void setup(void)
> > shm_size = hpage_size * hugepages / 2;
> > update_shm_size(&shm_size);
> > shmkey = getipckey();
> > -
> > - /* initialize stat_time */
> > - stat_time = FIRST;
> > -
> > - /*
> > - * Create a shared memory segment with read and write
> > - * permissions. Do this here instead of in setup()
> > - * so that looping (-i) will work correctly.
> > - */
> > - shm_id_1 = shmget(shmkey, shm_size,
> > - SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
> > - if (shm_id_1 == -1)
> > - tst_brk(TBROK | TERRNO, "shmget #main");
> > }
> >
> > void cleanup(void)
> >
>
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
--
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200306/4511323d/attachment-0001.htm>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [LTP] [PATCH 1/1] hugeshmctl01: Fix reset stat_time when looping with -i n
2020-03-06 5:43 ` Li Wang
@ 2020-03-06 6:12 ` Yang Xu
0 siblings, 0 replies; 5+ messages in thread
From: Yang Xu @ 2020-03-06 6:12 UTC (permalink / raw)
To: ltp
Hi
>
>
> On Fri, Mar 6, 2020 at 9:53 AM Yang Xu <xuyang2018.jy@cn.fujitsu.com
> <mailto:xuyang2018.jy@cn.fujitsu.com>> wrote:
>
> Hi Petr
>
> > c7a2d296b didn't reset stat_time, thus uninitialized set_shared was
> > assigned to test variable and test got value from a null pointer,
> > which leaded to segfault.
> >
> > hugeshmctl01.c:279: PASS: shmctl in func_rmid() failed as
> expected, shared memory appears to be removed
> > tst_checkpoint.c:147: BROK: hugeshmctl01.c:152:
> tst_checkpoint_wait(0, 10000): ETIMEDOUT (110)
> > mem.c:817: INFO: set nr_hugepages to 0
> >
> > dmesg:
> > segfault at 7f73f8c00000 ip 00000000004051e1 sp 00007ffef375f9a0
> error 6 in hugeshmctl01.master[404000+13000]
> > addr2line -e hugeshmctl01 -f? 00000000004051e1
> > stat_setup
> > hugeshmctl01.c:139 (discriminator 4)
> >
> > test = (stat_time == FIRST) ? set_shmat() : set_shared;
> >
> > /* do an assignement for fun */
> > *(int *)test = i; // error here
> >
> > Fixes: c7a2d296b ("hugeshmctl: Use loop from the API")
> >
> > Reported-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com
> <mailto:xuyang2018.jy@cn.fujitsu.com>>
> > Suggested-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com
> <mailto:xuyang2018.jy@cn.fujitsu.com>>
> > Signed-off-by: Petr Vorel <pvorel@suse.cz <mailto:pvorel@suse.cz>>
> > ---
> > Hi Xu,
> >
> > I'm sorry for introducing a regression.
> > Thank you for a report and fixing the test.
> > I'd personally prefer to keep .tcnt = ARRAY_SIZE(tcases),
> > but maybe others will prefer to keep loop in test_hugeshmctl()
> > as it was before my change.
> >
> > BTW it'd be nice to have something like setup_i() and cleanup_i(),
> > which would be called before/after each run of whole test, when
> run with
> > -i n.
> Sub tests have own clean and setup function. They only reused? a System
> V shared memory segment. IMO, this is a question of coupling.
> >
> > Kind regards,
> > Petr
> >
> >? ?.../mem/hugetlb/hugeshmctl/hugeshmctl01.c? ? ?| 27
> ++++++++++---------
> >? ?1 file changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git
> a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> > index e6cf8bf09..3b7e14363 100644
> > --- a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> > +++ b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> > @@ -75,6 +75,20 @@ struct tcase {
> >
> >? ?static void test_hugeshmctl(unsigned int i)
> >? ?{
> > +? ? ?stat_time = FIRST;
> > +
> My description may confuse you.? stat_time should not be reseted every
> time, it only needs to be reseted when next loop. This value will be +1
> ? when call stat_cleanup.
>
>
> Xu Yang is correct here. If we do reset 'stat_time' to FIRST in each
> loop then it would be missing?the SECOND part in the stat_setup(). We
> can fix the problem simply to move the 'stat_time' to if-condition as he
> suggested.
>
> But to be honest, the looping workflow of hugeshmctl01?looks a little
> bit in disorder that may be the reason makes Petr missed the 'stat_time'
> value part, I?appreciated?if someone who can help to refactor the
> hugeshmctl01:).
I guess we can assign value for stat_time directly and remove
stat_time++ in stat_cleanup as xiao suggested. Also, to remove the
coupling of this four cases, every loop recreating a shared memeory is
ok for me. So we can run single test case instead of 4 cases if we add a
parameter in the future.
like:
-N Execute the Nth test
-R Random the test
./hugeshmctl01 -N 2
Best Regards
Yang Xu
>
> struct tcase {
> ? ? ? ? ?int cmd;
> ? ? ? ? ?void (*func_test) (void);
> ? ? ? ? ?void (*func_setup) (void);
> } tcases[] = {
> ? ? ? ? ?{IPC_STAT, func_stat, stat_setup},? ?//stat_time = FIRST
> ? ? ? ? ?{IPC_STAT, func_stat, stat_setup},? ?//stat_time = SECOND
>
> As you do, the first and second case are same. it should be added into
> the "if == 0".
>
> ps: I personally think old case is more cleaner. Let's hear from others.
>
> Best Regards
> Yang Xu
> > +? ? ?/*
> > +? ? ? * Create a shared memory segment with read and write
> > +? ? ? * permissions. Do this here instead of in setup()
> > +? ? ? * so that looping (-i) will work correctly.
> > +? ? ? */
> > +? ? ?if (i == 0) {
> > +? ? ? ? ? ? ?shm_id_1 = shmget(shmkey, shm_size,
> > +? ? ? ? ? ? ? ? ? ? ? ? ? ? ?SHM_HUGETLB | IPC_CREAT | IPC_EXCL
> | SHM_RW);
> > +? ? ? ? ? ? ?if (shm_id_1 == -1)
> > +? ? ? ? ? ? ? ? ? ? ?tst_brk(TBROK | TERRNO, "shmget #main");
> > +? ? ?}
> > +
> >? ? ? ?/*
> >? ? ? ? * if needed, set up any required conditions by
> >? ? ? ? * calling the appropriate setup function
> > @@ -296,19 +310,6 @@ void setup(void)
> >? ? ? ?shm_size = hpage_size * hugepages / 2;
> >? ? ? ?update_shm_size(&shm_size);
> >? ? ? ?shmkey = getipckey();
> > -
> > -? ? ?/* initialize stat_time */
> > -? ? ?stat_time = FIRST;
> > -
> > -? ? ?/*
> > -? ? ? * Create a shared memory segment with read and write
> > -? ? ? * permissions.? Do this here instead of in setup()
> > -? ? ? * so that looping (-i) will work correctly.
> > -? ? ? */
> > -? ? ?shm_id_1 = shmget(shmkey, shm_size,
> > -? ? ? ? ? ? ? ? ? ? ?SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
> > -? ? ?if (shm_id_1 == -1)
> > -? ? ? ? ? ? ?tst_brk(TBROK | TERRNO, "shmget #main");
> >? ?}
> >
> >? ?void cleanup(void)
> >
>
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
>
> --
> Regards,
> Li Wang
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-06 6:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-05 17:18 [LTP] [PATCH 1/1] hugeshmctl01: Fix reset stat_time when looping with -i n Petr Vorel
2020-03-06 1:53 ` Yang Xu
2020-03-06 3:29 ` Xiao Yang
2020-03-06 5:43 ` Li Wang
2020-03-06 6:12 ` Yang Xu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.