| Message ID | 20181102025321.25429-2-honli@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] complib/cl_event_wheel.h improve comment documentation | expand |
On 11/1/2018 10:53 PM, Honggang LI wrote: > From: Honggang Li <honli@redhat.com> > > Signed-off-by: Honggang Li <honli@redhat.com> > --- > complib/cl_event_wheel.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/complib/cl_event_wheel.c b/complib/cl_event_wheel.c > index 27443f66..21e1f4ee 100644 > --- a/complib/cl_event_wheel.c > +++ b/complib/cl_event_wheel.c > @@ -457,6 +457,7 @@ uint32_t cl_event_wheel_num_regs(IN cl_event_wheel_t * const p_event_wheel, > } > > #ifdef __CL_EVENT_WHEEL_TEST__ > +#include <unistd.h> /* sleep() */ > > /* Dump out the complete state of the event wheel */ > void __cl_event_wheel_dump(IN cl_event_wheel_t * const p_event_wheel) > @@ -511,14 +512,10 @@ static uint64_t __test_event_aging(uint64_t key, uint32_t num_regs, void *contex > int main() > { > cl_event_wheel_t event_wheel; > - /* uint64_t key; */ > > /* init complib */ > complib_init(); > > - /* construct */ > - cl_event_wheel_construct(&event_wheel); > - While this is redundant, it breaks the general design pattern. Should it be changed ? Does it make some difference ? Note that event wheel is used for SM trap handling and also to support now "ancient" vendor driver (for generation 1 prior to OpenIB/OpenFabrics). > /* init */ > cl_event_wheel_init(&event_wheel); > > @@ -534,7 +531,7 @@ int main() > "The Second Aging Event"); > > cl_event_wheel_reg(&event_wheel, 3, /* key */ > - cl_get_time_stamp() + 3500000, /* 3 sec lifetime */ > + cl_get_time_stamp() + 3500000, /* 3.5 sec lifetime */ > __test_event_aging, /* cb */ > "The Third Aging Event"); > > @@ -542,7 +539,7 @@ int main() > > sleep(2); > cl_event_wheel_reg(&event_wheel, 2, /* key */ > - cl_get_time_stamp() + 8000000, /* 3 sec lifetime */ > + cl_get_time_stamp() + 8000000, /* 8 sec lifetime */ > __test_event_aging, /* cb */ > "The Second Aging Event Moved"); > >
On Fri, Nov 02, 2018 at 08:40:40AM -0400, Hal Rosenstock wrote: > On 11/1/2018 10:53 PM, Honggang LI wrote: > > From: Honggang Li <honli@redhat.com> > > > > Signed-off-by: Honggang Li <honli@redhat.com> > > --- > > complib/cl_event_wheel.c | 9 +++------ > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/complib/cl_event_wheel.c b/complib/cl_event_wheel.c > > index 27443f66..21e1f4ee 100644 > > --- a/complib/cl_event_wheel.c > > +++ b/complib/cl_event_wheel.c > > @@ -457,6 +457,7 @@ uint32_t cl_event_wheel_num_regs(IN cl_event_wheel_t * const p_event_wheel, > > } > > > > #ifdef __CL_EVENT_WHEEL_TEST__ > > +#include <unistd.h> /* sleep() */ > > > > /* Dump out the complete state of the event wheel */ > > void __cl_event_wheel_dump(IN cl_event_wheel_t * const p_event_wheel) > > @@ -511,14 +512,10 @@ static uint64_t __test_event_aging(uint64_t key, uint32_t num_regs, void *contex > > int main() > > { > > cl_event_wheel_t event_wheel; > > - /* uint64_t key; */ > > > > /* init complib */ > > complib_init(); > > > > - /* construct */ > > - cl_event_wheel_construct(&event_wheel); > > - > > While this is redundant, it breaks the general design pattern. Should it > be changed ? Does it make some difference ? The cl_event_wheel_init duplicates the works for cl_event_wheel_construct. If you want keep the general design pattern, you need update cl_event_wheel_init to call cl_event_wheel_construct. > > Note that event wheel is used for SM trap handling and also to support > now "ancient" vendor driver (for generation 1 prior to OpenIB/OpenFabrics). > > > /* init */ > > cl_event_wheel_init(&event_wheel); > > > > @@ -534,7 +531,7 @@ int main() > > "The Second Aging Event"); > > > > cl_event_wheel_reg(&event_wheel, 3, /* key */ > > - cl_get_time_stamp() + 3500000, /* 3 sec lifetime */ > > + cl_get_time_stamp() + 3500000, /* 3.5 sec lifetime */ > > __test_event_aging, /* cb */ > > "The Third Aging Event"); > > > > @@ -542,7 +539,7 @@ int main() > > > > sleep(2); > > cl_event_wheel_reg(&event_wheel, 2, /* key */ > > - cl_get_time_stamp() + 8000000, /* 3 sec lifetime */ > > + cl_get_time_stamp() + 8000000, /* 8 sec lifetime */ > > __test_event_aging, /* cb */ > > "The Second Aging Event Moved"); > > > >
On 11/2/2018 9:11 AM, Honggang LI wrote: > On Fri, Nov 02, 2018 at 08:40:40AM -0400, Hal Rosenstock wrote: >> On 11/1/2018 10:53 PM, Honggang LI wrote: >>> From: Honggang Li <honli@redhat.com> >>> >>> Signed-off-by: Honggang Li <honli@redhat.com> >>> --- >>> complib/cl_event_wheel.c | 9 +++------ >>> 1 file changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/complib/cl_event_wheel.c b/complib/cl_event_wheel.c >>> index 27443f66..21e1f4ee 100644 >>> --- a/complib/cl_event_wheel.c >>> +++ b/complib/cl_event_wheel.c >>> @@ -457,6 +457,7 @@ uint32_t cl_event_wheel_num_regs(IN cl_event_wheel_t * const p_event_wheel, >>> } >>> >>> #ifdef __CL_EVENT_WHEEL_TEST__ >>> +#include <unistd.h> /* sleep() */ >>> >>> /* Dump out the complete state of the event wheel */ >>> void __cl_event_wheel_dump(IN cl_event_wheel_t * const p_event_wheel) >>> @@ -511,14 +512,10 @@ static uint64_t __test_event_aging(uint64_t key, uint32_t num_regs, void *contex >>> int main() >>> { >>> cl_event_wheel_t event_wheel; >>> - /* uint64_t key; */ >>> >>> /* init complib * >>> complib_init(); >>> >>> - /* construct */ >>> - cl_event_wheel_construct(&event_wheel); >>> - >> >> While this is redundant, it breaks the general design pattern. Should it >> be changed ? Does it make some difference ? > > The cl_event_wheel_init duplicates the works for cl_event_wheel_construct. That's what I meant by it being redundant. > If you want keep the general design pattern, you need update > cl_event_wheel_init to call cl_event_wheel_construct. Why ? It may have just been "optimized" to incorporate what was needed from construct in the init function. I don't see why this needs to change. >> >> Note that event wheel is used for SM trap handling and also to support >> now "ancient" vendor driver (for generation 1 prior to OpenIB/OpenFabrics). These use cases follow the design pattern of calling construct and then init. >> >>> /* init */ >>> cl_event_wheel_init(&event_wheel); >>> >>> @@ -534,7 +531,7 @@ int main() >>> "The Second Aging Event"); >>> >>> cl_event_wheel_reg(&event_wheel, 3, /* key */ >>> - cl_get_time_stamp() + 3500000, /* 3 sec lifetime */ >>> + cl_get_time_stamp() + 3500000, /* 3.5 sec lifetime */ >>> __test_event_aging, /* cb */ >>> "The Third Aging Event"); >>> >>> @@ -542,7 +539,7 @@ int main() >>> >>> sleep(2); >>> cl_event_wheel_reg(&event_wheel, 2, /* key */ >>> - cl_get_time_stamp() + 8000000, /* 3 sec lifetime */ >>> + cl_get_time_stamp() + 8000000, /* 8 sec lifetime */ >>> __test_event_aging, /* cb */ >>> "The Second Aging Event Moved"); >>> >>> >
On Fri, Nov 02, 2018 at 09:17:43AM -0400, Hal Rosenstock wrote: > On 11/2/2018 9:11 AM, Honggang LI wrote: > > On Fri, Nov 02, 2018 at 08:40:40AM -0400, Hal Rosenstock wrote: > >> On 11/1/2018 10:53 PM, Honggang LI wrote: > >>> From: Honggang Li <honli@redhat.com> > >>> > >>> Signed-off-by: Honggang Li <honli@redhat.com> > >>> --- > >>> complib/cl_event_wheel.c | 9 +++------ > >>> 1 file changed, 3 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/complib/cl_event_wheel.c b/complib/cl_event_wheel.c > >>> index 27443f66..21e1f4ee 100644 > >>> --- a/complib/cl_event_wheel.c > >>> +++ b/complib/cl_event_wheel.c > >>> @@ -457,6 +457,7 @@ uint32_t cl_event_wheel_num_regs(IN cl_event_wheel_t * const p_event_wheel, > >>> } > >>> > >>> #ifdef __CL_EVENT_WHEEL_TEST__ > >>> +#include <unistd.h> /* sleep() */ > >>> > >>> /* Dump out the complete state of the event wheel */ > >>> void __cl_event_wheel_dump(IN cl_event_wheel_t * const p_event_wheel) > >>> @@ -511,14 +512,10 @@ static uint64_t __test_event_aging(uint64_t key, uint32_t num_regs, void *contex > >>> int main() > >>> { > >>> cl_event_wheel_t event_wheel; > >>> - /* uint64_t key; */ > >>> > >>> /* init complib * > >>> complib_init(); > >>> > >>> - /* construct */ > >>> - cl_event_wheel_construct(&event_wheel); > >>> - > >> > >> While this is redundant, it breaks the general design pattern. Should it > >> be changed ? Does it make some difference ? > > > > The cl_event_wheel_init duplicates the works for cl_event_wheel_construct. > > That's what I meant by it being redundant. > > > If you want keep the general design pattern, you need update > > cl_event_wheel_init to call cl_event_wheel_construct. > > Why ? It may have just been "optimized" to incorporate what was needed > from construct in the init function. I don't see why this needs to change. Just scanned opensm code, all usage of cl_event_wheel_init_ex stick with cl_event_wheel_construct. OK. It seems your comments make sense. > > >> > >> Note that event wheel is used for SM trap handling and also to support > >> now "ancient" vendor driver (for generation 1 prior to OpenIB/OpenFabrics). > > These use cases follow the design pattern of calling construct and then > init. > > >> > >>> /* init */ > >>> cl_event_wheel_init(&event_wheel); > >>> > >>> @@ -534,7 +531,7 @@ int main() > >>> "The Second Aging Event"); > >>> > >>> cl_event_wheel_reg(&event_wheel, 3, /* key */ > >>> - cl_get_time_stamp() + 3500000, /* 3 sec lifetime */ > >>> + cl_get_time_stamp() + 3500000, /* 3.5 sec lifetime */ > >>> __test_event_aging, /* cb */ > >>> "The Third Aging Event"); > >>> > >>> @@ -542,7 +539,7 @@ int main() > >>> > >>> sleep(2); > >>> cl_event_wheel_reg(&event_wheel, 2, /* key */ > >>> - cl_get_time_stamp() + 8000000, /* 3 sec lifetime */ > >>> + cl_get_time_stamp() + 8000000, /* 8 sec lifetime */ > >>> __test_event_aging, /* cb */ > >>> "The Second Aging Event Moved"); > >>> > >>> > >
On 11/2/2018 9:37 AM, Honggang LI wrote: > On Fri, Nov 02, 2018 at 09:17:43AM -0400, Hal Rosenstock wrote: >> On 11/2/2018 9:11 AM, Honggang LI wrote: >>> On Fri, Nov 02, 2018 at 08:40:40AM -0400, Hal Rosenstock wrote: >>>> On 11/1/2018 10:53 PM, Honggang LI wrote: >>>>> From: Honggang Li <honli@redhat.com> >>>>> >>>>> Signed-off-by: Honggang Li <honli@redhat.com> >>>>> --- >>>>> complib/cl_event_wheel.c | 9 +++------ >>>>> 1 file changed, 3 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/complib/cl_event_wheel.c b/complib/cl_event_wheel.c >>>>> index 27443f66..21e1f4ee 100644 >>>>> --- a/complib/cl_event_wheel.c >>>>> +++ b/complib/cl_event_wheel.c >>>>> @@ -457,6 +457,7 @@ uint32_t cl_event_wheel_num_regs(IN cl_event_wheel_t * const p_event_wheel, >>>>> } >>>>> >>>>> #ifdef __CL_EVENT_WHEEL_TEST__ >>>>> +#include <unistd.h> /* sleep() */ >>>>> >>>>> /* Dump out the complete state of the event wheel */ >>>>> void __cl_event_wheel_dump(IN cl_event_wheel_t * const p_event_wheel) >>>>> @@ -511,14 +512,10 @@ static uint64_t __test_event_aging(uint64_t key, uint32_t num_regs, void *contex >>>>> int main() >>>>> { >>>>> cl_event_wheel_t event_wheel; >>>>> - /* uint64_t key; */ >>>>> >>>>> /* init complib * >>>>> complib_init(); >>>>> >>>>> - /* construct */ >>>>> - cl_event_wheel_construct(&event_wheel); >>>>> - >>>> >>>> While this is redundant, it breaks the general design pattern. Should it >>>> be changed ? Does it make some difference ? >>> >>> The cl_event_wheel_init duplicates the works for cl_event_wheel_construct. >> >> That's what I meant by it being redundant. >> >>> If you want keep the general design pattern, you need update >>> cl_event_wheel_init to call cl_event_wheel_construct. >> >> Why ? It may have just been "optimized" to incorporate what was needed >> from construct in the init function. I don't see why this needs to change. > > Just scanned opensm code, all usage of cl_event_wheel_init_ex stick with cl_event_wheel_construct. > > OK. It seems your comments make sense. Just pushed the remaining parts of these 2 patches. >> >>>> >>>> Note that event wheel is used for SM trap handling and also to support >>>> now "ancient" vendor driver (for generation 1 prior to OpenIB/OpenFabrics). >> >> These use cases follow the design pattern of calling construct and then >> init. >> >>>> >>>>> /* init */ >>>>> cl_event_wheel_init(&event_wheel); >>>>> >>>>> @@ -534,7 +531,7 @@ int main() >>>>> "The Second Aging Event"); >>>>> >>>>> cl_event_wheel_reg(&event_wheel, 3, /* key */ >>>>> - cl_get_time_stamp() + 3500000, /* 3 sec lifetime */ >>>>> + cl_get_time_stamp() + 3500000, /* 3.5 sec lifetime */ >>>>> __test_event_aging, /* cb */ >>>>> "The Third Aging Event"); >>>>> >>>>> @@ -542,7 +539,7 @@ int main() >>>>> >>>>> sleep(2); >>>>> cl_event_wheel_reg(&event_wheel, 2, /* key */ >>>>> - cl_get_time_stamp() + 8000000, /* 3 sec lifetime */ >>>>> + cl_get_time_stamp() + 8000000, /* 8 sec lifetime */ >>>>> __test_event_aging, /* cb */ >>>>> "The Second Aging Event Moved"); >>>>> >>>>> >>> >
diff --git a/complib/cl_event_wheel.c b/complib/cl_event_wheel.c index 27443f66..21e1f4ee 100644 --- a/complib/cl_event_wheel.c +++ b/complib/cl_event_wheel.c @@ -457,6 +457,7 @@ uint32_t cl_event_wheel_num_regs(IN cl_event_wheel_t * const p_event_wheel, } #ifdef __CL_EVENT_WHEEL_TEST__ +#include <unistd.h> /* sleep() */ /* Dump out the complete state of the event wheel */ void __cl_event_wheel_dump(IN cl_event_wheel_t * const p_event_wheel) @@ -511,14 +512,10 @@ static uint64_t __test_event_aging(uint64_t key, uint32_t num_regs, void *contex int main() { cl_event_wheel_t event_wheel; - /* uint64_t key; */ /* init complib */ complib_init(); - /* construct */ - cl_event_wheel_construct(&event_wheel); - /* init */ cl_event_wheel_init(&event_wheel); @@ -534,7 +531,7 @@ int main() "The Second Aging Event"); cl_event_wheel_reg(&event_wheel, 3, /* key */ - cl_get_time_stamp() + 3500000, /* 3 sec lifetime */ + cl_get_time_stamp() + 3500000, /* 3.5 sec lifetime */ __test_event_aging, /* cb */ "The Third Aging Event"); @@ -542,7 +539,7 @@ int main() sleep(2); cl_event_wheel_reg(&event_wheel, 2, /* key */ - cl_get_time_stamp() + 8000000, /* 3 sec lifetime */ + cl_get_time_stamp() + 8000000, /* 8 sec lifetime */ __test_event_aging, /* cb */ "The Second Aging Event Moved");