On 30 May 2016 at 14:12, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com> wrote: > > > > From: Christophe Milard [mailto:christophe.mil...@linaro.org] > Sent: Monday, May 30, 2016 1:34 PM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com> > Cc: yi...@linaro.org; lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCHv3] helper: cleaner interface to > odph_odpthreads_create/join > > > > On 30 May 2016 at 11:23, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolai...@nokia.com> wrote: > > > From: Christophe Milard [mailto:christophe.mil...@linaro.org] > Sent: Friday, May 27, 2016 4:08 PM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com> > Cc: yi...@linaro.org; lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCHv3] helper: cleaner interface to > odph_odpthreads_create/join > > > > On 27 May 2016 at 09:56, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolai...@nokia.com> wrote: > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of > > Christophe Milard > > Sent: Friday, May 27, 2016 10:39 AM > > To: yi...@linaro.org; lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCHv3] helper: cleaner interface to > > odph_odpthreads_create/join > > > > The odph_odpthread_t (non opaque array of odpthread) is now replaced by a > > single type odph_odpthread_tbl_t abstracted as a void*. > > The table describing the odpthreads being created is now malloc'd and > > freed by the helper function themselves. > > > > > This change assumes that application will create all threads with a single > call (which is not always practical or even possible). If application creates > threads one by one, you'd end up with N separate thread table pointers, which > application would need to store into another table anyway. > > Correct: My approach let tha application decides how it wants to group the > threads: If the threads share the same functions to run etc, the application > can handle it in a single call (1 table of N threads). If the threads cannot > be stardes at the same time, tha application my take the N table if 1 entry > instead. > > > It would be better to either define opaque odph_odpthread_t with correct > size, or define alloc/free calls for a odph_odpthread_tbl_t. > > odph_odpthread_tbl_t *thr_tbl; > int num = 5; > odp_cpumask_t cpumask; > > thr_tbl = odph_odpthread_tbl_alloc(num); > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 1); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > ... > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 2); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > ... > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 3); > odp_cpumask_set(&cpumask, 4); > odp_cpumask_set(&cpumask, 5); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > hmmm... what would happen at join time? would the application have to provide > a mask with the set of CPU threads to be join hence letting the > responsibility of the join consistency to the application? or would all the > threads in the table be joined (which is maybe not what the application > wants)?... > > What happens the day we say more than one thread can run on each cpu? > (the current approach can easily be modified by passing an array of N cpu > number -possibly including the same cpu many times-, instead of the current > mask at creation time.). How would you describe which threads should be > started or joined in your case? I guess an array of odp_thread ids would have > to be given as well... or? > > I think the appraoch taken here is cleaner: each table is a group of threads > which are created and joined at the same time. The helpers guarantees this > consistency over such a group: the table size, the starting of all group > threads and the joining of all group threads is done by the helper. > Yes, the application may have the need to create many such tables if it needs > different groups of threads, but the consistency within each group is > garanteed by the helper. > Besides, the need to have different tables for different group of threads > does not necessarily make the application itself less clear: the pktio > performance test, for instance, now defines two sets of threads: the TX > threads and the RX threads. Is that worse than having a single table of > threads and letting the application try to distinguish them by some other > means (the cpu mask in your approach, as long as this works...) > > Your approach removes the need to have many tables within the application, > indeed, but the consistency (size of table, number of started threads and > number of joined threads) has to be maintained by the caller. > My approach lets the helper maintain the consistency within a table and force > the application to separate different thread categories in different tables... > > I still think my approach is more consistent. Hope I can convince you :-) > > Have a nice week-end, > > Christophe. > > > Of course, I had a bug in my example above and missed the index into the > table... Logic is the same we already have in direct pthread/process calls > (provides the same flexibility). > > odph_odpthread_tbl_t *thr_tbl; > int num = 5; > odp_cpumask_t cpumask; > > thr_tbl = odph_odpthread_tbl_alloc(num); > > // create first > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 1); > odph_odpthreads_create(&thr_tbl[0], &cpumask, ...); > > ... > > // create second > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 2); > odph_odpthreads_create(&thr_tbl[1], &cpumask, ...); > > ... > > // create three more > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 3); > odp_cpumask_set(&cpumask, 4); > odp_cpumask_set(&cpumask, 5); > odph_odpthreads_create(&thr_tbl[2], &cpumask, ...); > > ... > > // wait for the first two > odph_odpthreads_join(&thr_tbl[0], 2); > > // wait for the rest > odph_odpthreads_join(&thr_tbl[2], 3); > > odph_odpthread_tbl_free(thr_tbl); > > > -Petri > That works, but is it really better? The applications are then forced to > handle implicit sub tables in this single table (making sure those entries > which have to be joined together are contiguous in the table). The > application has to handle that by itself, remembering at which index each sub > section starts and how many threads they includes. and allocating enough room > for all at start. > > The approach I took delegates all this to the helpers: your sub-tables are > forced into visibility (as separate table). Within each of these, the helpers > maintain consistency. > > I still don't see what makes your approach better. > > Christophe. > > > > No HTML mails, please ... the list converts HTML to plain text, but direct > HTML mail to me still messes up my client.
Sorry. Hope this is better now. > > > This approach is more flexible - application can decide how to manage the > table(s). If application wants, it can also create N tables and save those N > pointers (into another table). > > > odph_odpthread_tbl_t *thr_tbl[3]; > odp_cpumask_t cpumask; > > thr_tbl[0] = odph_odpthread_tbl_alloc(1); > thr_tbl[1] = odph_odpthread_tbl_alloc(1); > thr_tbl[2] = odph_odpthread_tbl_alloc(3); > > > // create first > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 1); > odph_odpthreads_create(thr_tbl[0], &cpumask, ...); > > ... > > // create second > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 2); > odph_odpthreads_create(thr_tbl[1], &cpumask, ...); > > ... > > // create three more > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 3); > odp_cpumask_set(&cpumask, 4); > odp_cpumask_set(&cpumask, 5); > odph_odpthreads_create(thr_tbl[2], &cpumask, ...); > > ... > > // wait for the first > odph_odpthreads_join(thr_tbl[0], 1); > > // wait for the second > odph_odpthreads_join(thr_tbl[1], 1); > > // wait for the rest > odph_odpthreads_join(thr_tbl[2], 3); > > odph_odpthread_tbl_free(thr_tbl[0]); > odph_odpthread_tbl_free(thr_tbl[1]); > odph_odpthread_tbl_free(thr_tbl[2]); > > > I don't see why inflexibility would be better than flexibility here. confused. What is inflexible? Using my original approach, the application is given the chance to delegate the table consistency to the helpers. If it doesn't want to do so, it can create N tables of 1 entry, and handle things manually. There are actually examples of both ways in the different tests. Your approach forces the manual approach only, as far as I can see. Any other voices to comment on this? Yi? Others? Christophe. > > > -Petri > > _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
