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. >
