1. Prefix has to be "PATCH API-NEXT" not "API-NEXT" 2. Please remove ifdefs. And compile all variants of schedules into the library. Code needs to be compile each time.
Maxim. On 29 March 2017 at 06:30, Bill Fischofer <[email protected]> wrote: > On Tue, Mar 28, 2017 at 10:28 PM, Honnappa Nagarahalli < > [email protected]> wrote: > > > I had run the checkpatch.pl, but I did not see these issues. May be it > > was to do with the way I generated the patch. I will address the ones > that > > make sense. > > > > I did a git am for your patches and then regenerated them for checkpatch > testing via: > git format-patch origin/api-next --subject-prefix="API-NEXT PATCH" > > > > > > Do we have to compile for 32b with gcc? > > > > Yes, we require that ODP code compile and run in both 32-bit and 64-bit > mode. > > > > > > From: Bill Fischofer [mailto:[email protected]] > > Sent: Tuesday, March 28, 2017 10:01 PM > > To: Brian Brooks > > Cc: lng-odp-forward; Ola Liljedahl; Kevin Wang; Honnappa Nagarahalli > > Subject: Re: [lng-odp] [API-NEXT 4/4] A scalable software scheduler > > > > First off, I'm not sure why you're seeing HTML since Gmail is set to > plain > > text mode for me. > > > > On Tue, Mar 28, 2017 at 9:18 PM, Brian Brooks <[email protected]> > > wrote: > > On 03/28 18:50:32, Bill Fischofer wrote: > > > <html><head><meta http-equiv="Content-Type" content="text/html; > > charset=utf-8"> > > > <meta name="Generator" content="Microsoft Exchange Server"> > > > <!-- converted from text --> > > > <style><!-- .EmailQuote { margin-left: 1pt; padding-left: 4pt; > > border-left: #800000 2px solid; } --></style></head> > > > <body> > > > <font size="2"><span style="font-size:10pt;"><div > class="PlainText">This > > part generates numerous checkpatch warnings and errors. Please<br> > > > run checkpatch and correct for v2.<br> > > > > We ran checkpatch.pl and corrected the issues that made sense. We all > > know that > > checkpatch.pl is not perfect. Please point out the checkpatch.pl issues > > that > > are generated from this patch which are not false positives or not > > destructive > > to programmer readability. > > > > Checkpatch does issue a number of spurious warnings relating to the use > of > > volatile, etc. but the following seem legit: > > > > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars > > per line) > > #11: > > respected, and (W)RR scheduling may be used within queues of the same > > priority. > > > > WARNING: 'absense' may be misspelled - perhaps 'absence'? > > #16: > > concurrent queues. LL/SC and CAS variants exist in cases where absense of > > > > CHECK: No space is necessary after a cast > > #171: FILE: platform/linux-generic/include/odp/api/plat/schedule_ > > types.h:65: > > +#define ODP_SCHED_GROUP_INVALID ((odp_schedule_group_t) -1) > > > > CHECK: Alignment should match open parenthesis > > #225: FILE: platform/linux-generic/include/odp_atomic16.h:45: > > +static inline bool __atomic_compare_exchange_16(register __int128 *var, > > +__int128 *exp, register __int128 neu, > > > > CHECK: Alignment should match open parenthesis > > #271: FILE: platform/linux-generic/include/odp_atomic16.h:91: > > +static inline bool __atomic_compare_exchange_16_frail(register __int128 > > *var, > > +__int128 *exp, register __int128 neu, bool weak, > > > > CHECK: Alignment should match open parenthesis > > #393: FILE: platform/linux-generic/include/odp_atomic16.h:213: > > +static inline __int128 __atomic_fetch_or_16(__int128 *var, > > +__int128 mask, > > > > CHECK: multiple assignments should be avoided > > #856: FILE: platform/linux-generic/include/odp_llqueue.h:123: > > +llq->u.st.head = llq->u.st.tail = node; > > > > CHECK: multiple assignments should be avoided > > #965: FILE: platform/linux-generic/include/odp_llqueue.h:232: > > +llq->u.st.head = llq->u.st.tail = NULL; > > > > CHECK: multiple assignments should be avoided > > #1064: FILE: platform/linux-generic/include/odp_llqueue.h:331: > > +llq->u.st.head = llq->u.st.tail = NULL; > > > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #1189: FILE: platform/linux-generic/include/odp_llsc.h:64: > > +} > > +#define ll32(a, b) ll((a), (b)) > > > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #1205: FILE: platform/linux-generic/include/odp_llsc.h:80: > > +} > > +#define sc32(a, b, c) sc((a), (b), (c)) > > > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #1220: FILE: platform/linux-generic/include/odp_llsc.h:95: > > +} > > +#define ll64(a, b) lld((a), (b)) > > > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #1236: FILE: platform/linux-generic/include/odp_llsc.h:111: > > +} > > +#define sc64(a, b, c) scd((a), (b), (c)) > > > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #1336: FILE: platform/linux-generic/include/odp_llsc.h:211: > > +} > > +#define ll64(a, b) ll((a), (b)) > > > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #1357: FILE: platform/linux-generic/include/odp_llsc.h:232: > > +} > > +#define sc64(a, b, c) sc((a), (b), (c)) > > > > > > CHECK: Alignment should match open parenthesis > > #1981: FILE: platform/linux-generic/include/odp_schedule_ordered_ > > internal.h:131: > > +void olock_unlock(const reorder_context_t *rctx, reorder_window_t *rwin, > > + uint32_t lock_index); > > > > > > WARNING: 'noone' may be misspelled - perhaps 'no one'? > > #2402: FILE: platform/linux-generic/odp_queue_scalable.c:95: > > +/* All remaining elements claimed, noone else can enqueue */ > > > > CHECK: spaces preferred around that '<<' (ctx:VxV) > > #2417: FILE: platform/linux-generic/odp_queue_scalable.c:110: > > +return 1<<i; > > ^ > > > > CHECK: Alignment should match open parenthesis > > #2438: FILE: platform/linux-generic/odp_queue_scalable.c:131: > > +shm = odp_shm_reserve(name, > > +ring_size * sizeof(odp_buffer_hdr_t *), > > > > CHECK: Alignment should match open parenthesis > > #2698: FILE: platform/linux-generic/odp_queue_scalable.c:391: > > +while (__atomic_load_n(&q->qschst.numevts, __ATOMIC_RELAXED) != 0 || > > +__atomic_load_n(&q->qschst.cur_ticket, __ATOMIC_RELAXED) != > > > > > > WARNING: macros should not use a trailing semicolon > > #3366: FILE: platform/linux-generic/odp_schedule_scalable.c:55: > > +#define __atomic_fetch_max(var, v, mo) do { \ > > +/* Evalulate 'v' once */ \ > > +__typeof__(v) tmp_v = (v); \ > > +__typeof__(*var) old_var = \ > > +__atomic_load_n((var), __ATOMIC_RELAXED); \ > > +while (tmp_v > old_var) { \ > > +/* Attempt to store 'v' in '*var' */ \ > > +if (__atomic_compare_exchange_n((var), &old_var, \ > > +tmp_v, true, (mo), \ > > +(mo))) \ > > +break; \ > > +/* Else failure, try again (with updated value of \ > > > > WARNING: Avoid unnecessary line continuations > > #3379: FILE: platform/linux-generic/odp_schedule_scalable.c:68: > > + */ \ > > > > CHECK: Alignment should match open parenthesis > > #3581: FILE: platform/linux-generic/odp_schedule_scalable.c:270: > > + LDXR8(&q->qschst.cur_ticket, > > + __ATOMIC_ACQUIRE) != ticket) > > > > CHECK: Alignment should match open parenthesis > > #3659: FILE: platform/linux-generic/odp_schedule_scalable.c:348: > > +static void sched_update_deq(sched_elem_t *q, > > +uint32_t actual, bool atomic) __attribute__((always_inline)); > > > > CHECK: Alignment should match open parenthesis > > #3661: FILE: platform/linux-generic/odp_schedule_scalable.c:350: > > +static inline void sched_update_deq(sched_elem_t *q, > > +uint32_t actual, bool atomic) > > > > CHECK: Alignment should match open parenthesis > > #3745: FILE: platform/linux-generic/odp_schedule_scalable.c:434: > > + LDXR8(&q->qschst.cur_ticket, > > + __ATOMIC_ACQUIRE) != ticket) > > > > CHECK: Alignment should match open parenthesis > > #3791: FILE: platform/linux-generic/odp_schedule_scalable.c:480: > > +static void sched_update_deq_sc(sched_elem_t *q, > > +uint32_t actual, bool atomic) __attribute__((always_inline)); > > > > CHECK: Alignment should match open parenthesis > > #3793: FILE: platform/linux-generic/odp_schedule_scalable.c:482: > > +static inline void sched_update_deq_sc(sched_elem_t *q, > > +uint32_t actual, bool atomic) > > > > CHECK: Alignment should match open parenthesis > > #3977: FILE: platform/linux-generic/odp_schedule_scalable.c:666: > > +sg_wanted = __atomic_load_n(&ts->sg_wanted[p], > > +__ATOMIC_ACQUIRE); > > > > CHECK: Alignment should match open parenthesis > > #3989: FILE: platform/linux-generic/odp_schedule_scalable.c:678: > > +insert_schedq_in_list(ts, > > +&sg->schedq[p * sg->xfactor + > > > > CHECK: Alignment should match open parenthesis > > #4002: FILE: platform/linux-generic/odp_schedule_scalable.c:691: > > +remove_schedq_from_list(ts, > > + &sg->schedq[p * sg->xfactor + x]); > > > > CHECK: multiple assignments should be avoided > > #4171: FILE: platform/linux-generic/odp_schedule_scalable.c:860: > > +ts->atomq = atomq = elem; > > > > CHECK: Alignment should match open parenthesis > > #4256: FILE: platform/linux-generic/odp_schedule_scalable.c:945: > > +if (odp_unlikely(__atomic_load_n(&rwin->turn, > > + __ATOMIC_ACQUIRE) != sn)) { > > > > > > CHECK: Alignment should match open parenthesis > > #4584: FILE: platform/linux-generic/odp_schedule_scalable.c:1273: > > +__atomic_store_n(&thread_state[thr].sg_sem, > > +1, > > > > CHECK: Alignment should match open parenthesis > > #4656: FILE: platform/linux-generic/odp_schedule_scalable.c:1345: > > +shm = odp_shm_reserve(name ? name : "", > > + (sizeof(sched_group_t) + > > > > CHECK: No space is necessary after a cast > > #4664: FILE: platform/linux-generic/odp_schedule_scalable.c:1353: > > +sg = (sched_group_t *) odp_shm_addr(shm); > > > > CHECK: No space is necessary after a cast > > #4684: FILE: platform/linux-generic/odp_schedule_scalable.c:1373: > > +return (odp_schedule_group_t) (sgi); > > > > CHECK: Alignment should match open parenthesis > > #5054: FILE: platform/linux-generic/odp_schedule_scalable.c:1743: > > +if (odp_schedule_group_join(ODP_SCHED_GROUP_CONTROL, > > +&mask) != 0) { > > > > CHECK: Alignment should match open parenthesis > > #5060: FILE: platform/linux-generic/odp_schedule_scalable.c:1749: > > +if (odp_schedule_group_join(ODP_SCHED_GROUP_WORKER, > > +&mask) != 0) { > > > > CHECK: Alignment should match open parenthesis > > #5096: FILE: platform/linux-generic/odp_schedule_scalable.c:1785: > > +if (odp_schedule_group_leave(ODP_SCHED_GROUP_CONTROL, > > +&mask) != 0) > > > > CHECK: Alignment should match open parenthesis > > #5100: FILE: platform/linux-generic/odp_schedule_scalable.c:1789: > > +if (odp_schedule_group_leave(ODP_SCHED_GROUP_WORKER, > > +&mask) != 0) > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #5185: FILE: platform/linux-generic/odp_schedule_scalable.c:1874: > > +} > > +static int thr_rem(odp_schedule_group_t group, int thr) > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #5192: FILE: platform/linux-generic/odp_schedule_scalable.c:1881: > > +} > > +static int init_queue(uint32_t queue_index, > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #5200: FILE: platform/linux-generic/odp_schedule_scalable.c:1889: > > +} > > +static void destroy_queue(uint32_t queue_index) > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #5205: FILE: platform/linux-generic/odp_schedule_scalable.c:1894: > > +} > > +static int sched_queue(uint32_t queue_index) > > > > CHECK: Please use a blank line after function/struct/union/enum > > declarations > > #5211: FILE: platform/linux-generic/odp_schedule_scalable.c:1900: > > +} > > +static int ord_enq_multi(uint32_t queue_index, void *p_buf_hdr[], > > > > > > CHECK: No space is necessary after a cast > > #5314: FILE: platform/linux-generic/odp_schedule_scalable_ordered.c:38: > > +rwin = (reorder_window_t *) odp_shm_addr(*shm); > > > > CHECK: Alignment should match open parenthesis > > #5408: FILE: platform/linux-generic/odp_schedule_scalable_ordered.c:132: > > +if (__atomic_compare_exchange(&rwin->hc, > > +&old, /* Updated on failure */ > > > > CHECK: Please don't use multiple blank lines > > #5757: FILE: test/common_plat/performance/odp_sched_latency.c:824: > > > > + > > > > total: 0 errors, 54 warnings, 44 checks, 5742 lines checked > > > > NOTE: Ignored message types: BIT_MACRO COMPARISON_TO_NULL > > DEPRECATED_VARIABLE NEW_TYPEDEFS SPLIT_STRING SSCANF_TO_KSTRTO > > > > 0004-A-scalable-software-scheduler.patch has style problems, please > > review. > > > > If any of these errors are false positives, please report > > them to the maintainer, see CHECKPATCH in MAINTAINERS. > > > > > > > <br> > > > Also, this part introduces a number of errors that result in > failure<br> > > > to compile using clang. > > > > This is likely true. Is compilation with Clang a blocker? > > > > Yes, it is. We require ODP modules to be compilable with either GCC or > > clang. > > > > > > IMPORTANT NOTICE: The contents of this email and any attachments are > > confidential and may also be privileged. If you are not the intended > > recipient, please notify the sender immediately and do not disclose the > > contents to any other person, use it for any purpose, or store or copy > the > > information in any medium. Thank you. > > >
