| Message ID | 1470839034-19332-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | Accepted |
| Commit | 35af07b2469e9f41769648b2d17b7d2be52ee270 |
| Headers | show |
I'm going to apply this patch to master. But test might fail on bug in term code. But I think we will have more eyes seeing this issue... If any objections please say asap. Maxim. On 08/10/16 17:23, Bill Fischofer wrote: > Accumulate return codes from resource cleanup so callers can be aware of > termination failures. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > test/common_plat/performance/odp_scheduling.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/test/common_plat/performance/odp_scheduling.c b/test/common_plat/performance/odp_scheduling.c > index 1d1bf01..1de79f7 100644 > --- a/test/common_plat/performance/odp_scheduling.c > +++ b/test/common_plat/performance/odp_scheduling.c > @@ -965,15 +965,15 @@ int main(int argc, char *argv[]) > > for (j = 0; j < QUEUES_PER_PRIO; j++) { > queue = globals->queue[i][j]; > - odp_queue_destroy(queue); > + ret += odp_queue_destroy(queue); > } > } > > - odp_shm_free(shm); > - odp_queue_destroy(plain_queue); > - odp_pool_destroy(pool); > - odp_term_local(); > - odp_term_global(instance); > + ret += odp_shm_free(shm); > + ret += odp_queue_destroy(plain_queue); > + ret += odp_pool_destroy(pool); > + ret += odp_term_local(); > + ret += odp_term_global(instance); > > return ret; > }
On 11 August 2016 at 13:01, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > I'm going to apply this patch to master. But test might fail on bug in > term code. But I think we will have more eyes seeing this issue... > If any objections please say asap. I object We have stated previously that no commit may ever break master becasue it can hamper git bisect, in addition I dont see any Reviews, so no one has had chance to take a proper look and sign off yet, it looks sensible to me but I have not reviewed it. No mature project should ever commit something that is not known benign just see what happens, downstream users should expect much more rigour from us. Can we point at a use case that is now fixed by this patch ? I think it needs reviews at a minimum. > > Maxim. > > > On 08/10/16 17:23, Bill Fischofer wrote: > >> Accumulate return codes from resource cleanup so callers can be aware of >> termination failures. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> test/common_plat/performance/odp_scheduling.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/test/common_plat/performance/odp_scheduling.c >> b/test/common_plat/performance/odp_scheduling.c >> index 1d1bf01..1de79f7 100644 >> --- a/test/common_plat/performance/odp_scheduling.c >> +++ b/test/common_plat/performance/odp_scheduling.c >> @@ -965,15 +965,15 @@ int main(int argc, char *argv[]) >> for (j = 0; j < QUEUES_PER_PRIO; j++) { >> queue = globals->queue[i][j]; >> - odp_queue_destroy(queue); >> + ret += odp_queue_destroy(queue); >> } >> } >> - odp_shm_free(shm); >> - odp_queue_destroy(plain_queue); >> - odp_pool_destroy(pool); >> - odp_term_local(); >> - odp_term_global(instance); >> + ret += odp_shm_free(shm); >> + ret += odp_queue_destroy(plain_queue); >> + ret += odp_pool_destroy(pool); >> + ret += odp_term_local(); >> + ret += odp_term_global(instance); >> return ret; >> } >> > > -- Mike Holmes Technical Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
On Thu, Aug 11, 2016 at 1:15 PM, Mike Holmes <mike.holmes@linaro.org> wrote: > On 11 August 2016 at 13:01, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > > I'm going to apply this patch to master. But test might fail on bug in > > term code. But I think we will have more eyes seeing this issue... > > If any objections please say asap. > > > I object > > We have stated previously that no commit may ever break master becasue it > can hamper git bisect, in addition I dont see any Reviews, so no one has > had chance to take a proper look and sign off yet, it looks sensible to me > but I have not reviewed it. > > No mature project should ever commit something that is not known benign > just see what happens, downstream users should expect much more rigour from > us. Can we point at a use case that is now fixed by this patch ? I think it > needs reviews at a minimum. > > I agree this should go through the normal review process. > > > > > > Maxim. > > > > > > On 08/10/16 17:23, Bill Fischofer wrote: > > > >> Accumulate return codes from resource cleanup so callers can be aware of > >> termination failures. > >> > >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > >> --- > >> test/common_plat/performance/odp_scheduling.c | 12 ++++++------ > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/test/common_plat/performance/odp_scheduling.c > >> b/test/common_plat/performance/odp_scheduling.c > >> index 1d1bf01..1de79f7 100644 > >> --- a/test/common_plat/performance/odp_scheduling.c > >> +++ b/test/common_plat/performance/odp_scheduling.c > >> @@ -965,15 +965,15 @@ int main(int argc, char *argv[]) > >> for (j = 0; j < QUEUES_PER_PRIO; j++) { > >> queue = globals->queue[i][j]; > >> - odp_queue_destroy(queue); > >> + ret += odp_queue_destroy(queue); > >> } > >> } > >> - odp_shm_free(shm); > >> - odp_queue_destroy(plain_queue); > >> - odp_pool_destroy(pool); > >> - odp_term_local(); > >> - odp_term_global(instance); > >> + ret += odp_shm_free(shm); > >> + ret += odp_queue_destroy(plain_queue); > >> + ret += odp_pool_destroy(pool); > >> + ret += odp_term_local(); > >> + ret += odp_term_global(instance); > >> return ret; > >> } > >> > > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows" >
On 08/11/16 21:43, Bill Fischofer wrote: > > On Thu, Aug 11, 2016 at 1:15 PM, Mike Holmes <mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>> wrote: > > On 11 August 2016 at 13:01, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > > I'm going to apply this patch to master. But test might fail on > bug in > > term code. But I think we will have more eyes seeing this issue... > > If any objections please say asap. > > > I object > > We have stated previously that no commit may ever break master > becasue it > can hamper git bisect, in addition I dont see any Reviews, so no > one has > had chance to take a proper look and sign off yet, it looks > sensible to me > but I have not reviewed it. > > No mature project should ever commit something that is not known > benign > just see what happens, downstream users should expect much more > rigour from > us. Can we point at a use case that is now fixed by this patch ? I > think it > needs reviews at a minimum. > > > I agree this should go through the normal review process. Just to be clear. There is now some bug which needs to be fixed. This patch if applied will show existence bug (somewhere in scheduler or pool code.). We should not introduce new bugs but also we should not "hide" existence and if test does not capture return codes than it has to be fixed. Because I was unsure if it's better to apply it or fix hidden issue and apply after I asked this question. Patch itself is good, I did my review and well it's very simple and there was enough time in ML for community review. Let's wait with this inclusion and spend some time more on debugging "hidden" issue. Maxim. > > > > > > Maxim. > > > > > > On 08/10/16 17:23, Bill Fischofer wrote: > > > >> Accumulate return codes from resource cleanup so callers can be > aware of > >> termination failures. > >> > >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> > >> --- > >> test/common_plat/performance/odp_scheduling.c | 12 ++++++------ > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/test/common_plat/performance/odp_scheduling.c > >> b/test/common_plat/performance/odp_scheduling.c > >> index 1d1bf01..1de79f7 100644 > >> --- a/test/common_plat/performance/odp_scheduling.c > >> +++ b/test/common_plat/performance/odp_scheduling.c > >> @@ -965,15 +965,15 @@ int main(int argc, char *argv[]) > >> for (j = 0; j < QUEUES_PER_PRIO; j++) { > >> queue = globals->queue[i][j]; > >> - odp_queue_destroy(queue); > >> + ret += odp_queue_destroy(queue); > >> } > >> } > >> - odp_shm_free(shm); > >> - odp_queue_destroy(plain_queue); > >> - odp_pool_destroy(pool); > >> - odp_term_local(); > >> - odp_term_global(instance); > >> + ret += odp_shm_free(shm); > >> + ret += odp_queue_destroy(plain_queue); > >> + ret += odp_pool_destroy(pool); > >> + ret += odp_term_local(); > >> + ret += odp_term_global(instance); > >> return ret; > >> } > >> > > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for > ARM SoCs > "Work should be fun and collaborative, the rest follows" > >
On 08/11/16 21:43, Bill Fischofer wrote: > > On Thu, Aug 11, 2016 at 1:15 PM, Mike Holmes <mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>> wrote: > > On 11 August 2016 at 13:01, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > > I'm going to apply this patch to master. But test might fail on > bug in > > term code. But I think we will have more eyes seeing this issue... > > If any objections please say asap. > > > I object > > We have stated previously that no commit may ever break master > becasue it > can hamper git bisect, in addition I dont see any Reviews, so no > one has > had chance to take a proper look and sign off yet, it looks > sensible to me > but I have not reviewed it. > > No mature project should ever commit something that is not known > benign > just see what happens, downstream users should expect much more > rigour from > us. Can we point at a use case that is now fixed by this patch ? I > think it > needs reviews at a minimum. > > > I agree this should go through the normal review process. Just to be clear. There is now some bug which needs to be fixed. This patch if applied will show existence bug (somewhere in scheduler or pool code.). We should not introduce new bugs but also we should not "hide" existence and if test does not capture return codes than it has to be fixed. Because I was unsure if it's better to apply it or fix hidden issue and apply after I asked this question. Patch itself is good, I did my review and well it's very simple and there was enough time in ML for community review. Let's wait with this inclusion and spend some time more on debugging "hidden" issue. Maxim. > > > > > > Maxim. > > > > > > On 08/10/16 17:23, Bill Fischofer wrote: > > > >> Accumulate return codes from resource cleanup so callers can be > aware of > >> termination failures. > >> > >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> > >> --- > >> test/common_plat/performance/odp_scheduling.c | 12 ++++++------ > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/test/common_plat/performance/odp_scheduling.c > >> b/test/common_plat/performance/odp_scheduling.c > >> index 1d1bf01..1de79f7 100644 > >> --- a/test/common_plat/performance/odp_scheduling.c > >> +++ b/test/common_plat/performance/odp_scheduling.c > >> @@ -965,15 +965,15 @@ int main(int argc, char *argv[]) > >> for (j = 0; j < QUEUES_PER_PRIO; j++) { > >> queue = globals->queue[i][j]; > >> - odp_queue_destroy(queue); > >> + ret += odp_queue_destroy(queue); > >> } > >> } > >> - odp_shm_free(shm); > >> - odp_queue_destroy(plain_queue); > >> - odp_pool_destroy(pool); > >> - odp_term_local(); > >> - odp_term_global(instance); > >> + ret += odp_shm_free(shm); > >> + ret += odp_queue_destroy(plain_queue); > >> + ret += odp_pool_destroy(pool); > >> + ret += odp_term_local(); > >> + ret += odp_term_global(instance); > >> return ret; > >> } > >> > > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for > ARM SoCs > "Work should be fun and collaborative, the rest follows" > >
This patch can go in now. Mike you want to add your review? Maxim. On 08/11/16 21:43, Bill Fischofer wrote: > > On Thu, Aug 11, 2016 at 1:15 PM, Mike Holmes <mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>> wrote: > > On 11 August 2016 at 13:01, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > > I'm going to apply this patch to master. But test might fail on > bug in > > term code. But I think we will have more eyes seeing this issue... > > If any objections please say asap. > > > I object > > We have stated previously that no commit may ever break master > becasue it > can hamper git bisect, in addition I dont see any Reviews, so no > one has > had chance to take a proper look and sign off yet, it looks > sensible to me > but I have not reviewed it. > > No mature project should ever commit something that is not known > benign > just see what happens, downstream users should expect much more > rigour from > us. Can we point at a use case that is now fixed by this patch ? I > think it > needs reviews at a minimum. > > > I agree this should go through the normal review process. > > > > > > > Maxim. > > > > > > On 08/10/16 17:23, Bill Fischofer wrote: > > > >> Accumulate return codes from resource cleanup so callers can be > aware of > >> termination failures. > >> > >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> > >> --- > >> test/common_plat/performance/odp_scheduling.c | 12 ++++++------ > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/test/common_plat/performance/odp_scheduling.c > >> b/test/common_plat/performance/odp_scheduling.c > >> index 1d1bf01..1de79f7 100644 > >> --- a/test/common_plat/performance/odp_scheduling.c > >> +++ b/test/common_plat/performance/odp_scheduling.c > >> @@ -965,15 +965,15 @@ int main(int argc, char *argv[]) > >> for (j = 0; j < QUEUES_PER_PRIO; j++) { > >> queue = globals->queue[i][j]; > >> - odp_queue_destroy(queue); > >> + ret += odp_queue_destroy(queue); > >> } > >> } > >> - odp_shm_free(shm); > >> - odp_queue_destroy(plain_queue); > >> - odp_pool_destroy(pool); > >> - odp_term_local(); > >> - odp_term_global(instance); > >> + ret += odp_shm_free(shm); > >> + ret += odp_queue_destroy(plain_queue); > >> + ret += odp_pool_destroy(pool); > >> + ret += odp_term_local(); > >> + ret += odp_term_global(instance); > >> return ret; > >> } > >> > > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for > ARM SoCs > "Work should be fun and collaborative, the rest follows" > >
Petris patch 5ed89eb fixed the real issue, and this patch does fix the code so that make check gets informed that it was failing. Reviewed-and-tested-by: Mike Holmes <mike.holmes@linaro.org> On 16 August 2016 at 09:10, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > This patch can go in now. Mike you want to add your review? > > Maxim. > > On 08/11/16 21:43, Bill Fischofer wrote: > >> >> On Thu, Aug 11, 2016 at 1:15 PM, Mike Holmes <mike.holmes@linaro.org >> <mailto:mike.holmes@linaro.org>> wrote: >> >> On 11 August 2016 at 13:01, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> > I'm going to apply this patch to master. But test might fail on >> bug in >> > term code. But I think we will have more eyes seeing this issue... >> > If any objections please say asap. >> >> >> I object >> >> We have stated previously that no commit may ever break master >> becasue it >> can hamper git bisect, in addition I dont see any Reviews, so no >> one has >> had chance to take a proper look and sign off yet, it looks >> sensible to me >> but I have not reviewed it. >> >> No mature project should ever commit something that is not known >> benign >> just see what happens, downstream users should expect much more >> rigour from >> us. Can we point at a use case that is now fixed by this patch ? I >> think it >> needs reviews at a minimum. >> >> >> I agree this should go through the normal review process. >> >> >> >> > >> > Maxim. >> > >> > >> > On 08/10/16 17:23, Bill Fischofer wrote: >> > >> >> Accumulate return codes from resource cleanup so callers can be >> aware of >> >> termination failures. >> >> >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>> >> >> >> --- >> >> test/common_plat/performance/odp_scheduling.c | 12 ++++++------ >> >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/test/common_plat/performance/odp_scheduling.c >> >> b/test/common_plat/performance/odp_scheduling.c >> >> index 1d1bf01..1de79f7 100644 >> >> --- a/test/common_plat/performance/odp_scheduling.c >> >> +++ b/test/common_plat/performance/odp_scheduling.c >> >> @@ -965,15 +965,15 @@ int main(int argc, char *argv[]) >> >> for (j = 0; j < QUEUES_PER_PRIO; j++) { >> >> queue = globals->queue[i][j]; >> >> - odp_queue_destroy(queue); >> >> + ret += odp_queue_destroy(queue); >> >> } >> >> } >> >> - odp_shm_free(shm); >> >> - odp_queue_destroy(plain_queue); >> >> - odp_pool_destroy(pool); >> >> - odp_term_local(); >> >> - odp_term_global(instance); >> >> + ret += odp_shm_free(shm); >> >> + ret += odp_queue_destroy(plain_queue); >> >> + ret += odp_pool_destroy(pool); >> >> + ret += odp_term_local(); >> >> + ret += odp_term_global(instance); >> >> return ret; >> >> } >> >> >> > >> > >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/> *│ *Open source software for >> ARM SoCs >> "Work should be fun and collaborative, the rest follows" >> >> >> > -- Mike Holmes Program Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
Merged, Maxim. On 08/17/16 21:05, Mike Holmes wrote: > Petris patch 5ed89eb fixed the real issue, and this patch does fix the > code so that make check gets informed that it was failing. > > Reviewed-and-tested-by: Mike Holmes <mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>> > > On 16 August 2016 at 09:10, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > This patch can go in now. Mike you want to add your review? > > Maxim. > > On 08/11/16 21:43, Bill Fischofer wrote: > > > On Thu, Aug 11, 2016 at 1:15 PM, Mike Holmes > <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org> > <mailto:mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>>> wrote: > > On 11 August 2016 at 13:01, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > > I'm going to apply this patch to master. But test might > fail on > bug in > > term code. But I think we will have more eyes seeing > this issue... > > If any objections please say asap. > > > I object > > We have stated previously that no commit may ever break master > becasue it > can hamper git bisect, in addition I dont see any Reviews, > so no > one has > had chance to take a proper look and sign off yet, it looks > sensible to me > but I have not reviewed it. > > No mature project should ever commit something that is not > known > benign > just see what happens, downstream users should expect much > more > rigour from > us. Can we point at a use case that is now fixed by this > patch ? I > think it > needs reviews at a minimum. > > > I agree this should go through the normal review process. > > > > > > > Maxim. > > > > > > On 08/10/16 17:23, Bill Fischofer wrote: > > > >> Accumulate return codes from resource cleanup so > callers can be > aware of > >> termination failures. > >> > >> Signed-off-by: Bill Fischofer > <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org> > <mailto:bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>>> > > >> --- > >> test/common_plat/performance/odp_scheduling.c | 12 > ++++++------ > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/test/common_plat/performance/odp_scheduling.c > >> b/test/common_plat/performance/odp_scheduling.c > >> index 1d1bf01..1de79f7 100644 > >> --- a/test/common_plat/performance/odp_scheduling.c > >> +++ b/test/common_plat/performance/odp_scheduling.c > >> @@ -965,15 +965,15 @@ int main(int argc, char *argv[]) > >> for (j = 0; j < QUEUES_PER_PRIO; j++) { > >> queue = globals->queue[i][j]; > >> - odp_queue_destroy(queue); > >> + ret += odp_queue_destroy(queue); > >> } > >> } > >> - odp_shm_free(shm); > >> - odp_queue_destroy(plain_queue); > >> - odp_pool_destroy(pool); > >> - odp_term_local(); > >> - odp_term_global(instance); > >> + ret += odp_shm_free(shm); > >> + ret += odp_queue_destroy(plain_queue); > >> + ret += odp_pool_destroy(pool); > >> + ret += odp_term_local(); > >> + ret += odp_term_global(instance); > >> return ret; > >> } > >> > > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source > software for > ARM SoCs > "Work should be fun and collaborative, the rest follows" > > > > > > > -- > Mike Holmes > Program Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows" >
diff --git a/test/common_plat/performance/odp_scheduling.c b/test/common_plat/performance/odp_scheduling.c index 1d1bf01..1de79f7 100644 --- a/test/common_plat/performance/odp_scheduling.c +++ b/test/common_plat/performance/odp_scheduling.c @@ -965,15 +965,15 @@ int main(int argc, char *argv[]) for (j = 0; j < QUEUES_PER_PRIO; j++) { queue = globals->queue[i][j]; - odp_queue_destroy(queue); + ret += odp_queue_destroy(queue); } } - odp_shm_free(shm); - odp_queue_destroy(plain_queue); - odp_pool_destroy(pool); - odp_term_local(); - odp_term_global(instance); + ret += odp_shm_free(shm); + ret += odp_queue_destroy(plain_queue); + ret += odp_pool_destroy(pool); + ret += odp_term_local(); + ret += odp_term_global(instance); return ret; }
Accumulate return codes from resource cleanup so callers can be aware of termination failures. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- test/common_plat/performance/odp_scheduling.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) -- 2.7.4