From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 6879B5A92 for ; Tue, 14 Apr 2015 17:29:23 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP; 14 Apr 2015 08:29:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,576,1422950400"; d="scan'208";a="555861240" Received: from bricha3-mobl3.ger.corp.intel.com ([10.243.20.33]) by orsmga003.jf.intel.com with SMTP; 14 Apr 2015 08:29:18 -0700 Received: by (sSMTP sendmail emulation); Tue, 14 Apr 2015 16:29:17 +0025 Date: Tue, 14 Apr 2015 16:29:17 +0100 From: Bruce Richardson To: Stephen Hemminger Message-ID: <20150414152916.GE3296@bricha3-MOBL3> References: <3571725.20GtF5MAnU@xps13> <0C5AFCA4B3408848ADF2A3073F7D8CC86D58F9C2@IRSMSX109.ger.corp.intel.com> <20150408111603.3497f306@urahara> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150408111603.3497f306@urahara> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] tools brainstorming X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Apr 2015 15:29:24 -0000 On Wed, Apr 08, 2015 at 11:16:03AM -0700, Stephen Hemminger wrote: > Thanks for doing this, it is a great start. > I admit strong bias towards Linux kernel style. > > Could you use one of the standard markup styles so that it could get put in documentation? > > > > License Header > > -------------- > > I prefer the file just say that it is BSD or GPL and refer to license files in the > package. That way if something has to change it doesn't need a massive license sweep > > > > > > Macros > > > > Do not ``#define`` or declare names in the implementation namespace except for implementing application interfaces. > > > > The names of ``unsafe`` macros (ones that have side effects), and the names of macros for manifest constants, are all in uppercase. > > > > The expansions of expression-like macros are either a single token or have outer parentheses. If a macro is an inline expansion of a function, > > the function name is all in lowercase and the macro has the same name all in uppercase. Right-justify the backslashes; > > it makes it easier to read. If the macro encapsulates a compound statement, enclose it in a do loop, so that it can be used safely in if statements. > > Any final statement-terminating semicolon should be supplied by the macro invocation rather than the macro, to make parsing easier for pretty-printers and editors. > > #define MACRO(x, y) do { \ > > variable = (x) + (y); \ > > (y) += 2; \ > > }while (0) > ^ bad whitespace > > it is important that all examples in documentation are perfect. > > > > C Function Definition, Declaration and Use > > > > Prototypes > > > > It is recommended, but not required that all functions are prototyped somewhere. > > > > Any function prototypes for private functions (that is, functions not used elsewhere) go at the top of the first source module. Functions > > local to one source module should be declared static. > > I find prototypes for private functions to be redundant and error prone. > The do nothing. Better to just put private functions in the correct order. > > > You also need to raise the issue that all global names need to be prefaced by a unique string. > I see places in drivers where global names leak out causing possible later name collision. > +1 to both. > > Definitions > > ----------- > > > > The function type should be on a line by itself preceding the function. The opening brace of the function body should be on a line by itself. > > static char * > > function(int a1, int a2, float fl, int a4) > > { > > Not a big fan of that style. Prefer it on same line. > > > > > > Indentation is a hard tab, that is, a tab character, not a sequence of spaces. > > Also no spaces before tabs. > > > NOTE General rule in DPDK, use tabs for indentation, spaces for alignment. > > If you have to wrap a long statement, put the operator at the end of the line, and indent again. For control statements (if, while, etc.), > > it is recommended that the next line be indented by two tabs, rather than one, to prevent confusion as to whether the second line of the > > control statement forms part of the statement body or not. For non-control statements, this issue does not apply, so they can be indented > > by a single tab. However, a two-tab indent is recommended in this case also to keep consistency across all statement types. > > while (really_long_variable_name_1 == really_long_variable_name_2 && > > var3 == var4){ > > x = y + z; /* control stmt body lines up with second line of */ > > a = b + c; /* control statement itself if single indent used */ > > } > > > > if (really_long_variable_name_1 == really_long_variable_name_2 && > > var3 == var4){ /* two tabs used */ > > No. Should line up with really_long_variable_name_1 > I disagree with that. For a couple of reasons: *. It means using spaces as well as tabs for indentation, while I think either one or the other should be used, not both. *. For anyone using a 4-character tab-stop display, the var3 line will line up visually with the body of the block making it look like the body, rather than part of the condition. For anyone using an 8-character tab, the same effect will be got with a while statement with a couple of opening braces. By using two tabs, we guarantee that the line continuation never lines up with the body of the block. > > x = y + z; /* statement body no longer lines up */ > > a = b + c; > > } > > > > z = a + really + long + statement + that + needs + > > two + lines + gets + indented + on + the + > > second + and + subsequent + lines; > > > > > > Do not add whitespace at the end of a line. > > > > Closing and opening braces go on the same line as the else keyword. Braces that are not necessary should be left out. > > if (test) > > stmt; > > else if (bar) { > > stmt; > > stmt; > > } else > > stmt; > > > > > > Function Calls > > -------------- > > > > Do not use spaces after function names. Commas should have a space after them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)`` characters. > > error = function(a1, a2); > > if (error != 0) > > exit(error); > > > > > > Operators > > --------- > > > > Unary operators do not require spaces, binary operators do. Do not use parentheses unless they are required for precedence or unless the > > statement is confusing without them. Remember that other people may be more easily confused than you. > > > > Exit > > > > Exits should be 0 on success, or 1 on failure. > > exit(0); /* > > * Avoid obvious comments such as > > * "Exit 0 on success." > > */ > > +11 > > > > > > Return Value > > ------------ > > > > If possible, functions should return 0 on success and a negative value on error. The negative value should be ``-errno`` if relevant, for example, ``-EINVAL``. > > > > Routines returning ``void *`` should not have their return values cast to any pointer type. > > (Typecasting can prevent the compiler from warning about missing prototypes as any implicit definition of a function returns int - which, unlike "void *" needs a typecast to assign to a pointer variable.) > > NOTE The above rule applies to malloc, as well as to DPDK functions. > > Values in return statements should be enclosed in parentheses. > > Sorry, this is a stupid BSDism > return (-EINVAL); > is ugly > Agree. > > > > Logging and Errors > > ------------------ > > > > In the DPDK environment, use the logging interface provided:: > > #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1 > > #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2 > > > > /* enable these logs type */ > > rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1); > > rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1); > > > > /* log in debug level */ > > rte_set_log_level(RTE_LOG_DEBUG); > > RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n"); > > RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n"); > > RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n"); > > > > /* log in info level */ > > rte_set_log_level(RTE_LOG_INFO); > > RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n"); > > > > > > In a userland program that is not a DPDK application, use err(3) or warn(3). Do not create your own variant. > > if ((four = malloc(sizeof(struct foo))) == NULL) > > err(1, (char *)NULL); > > if ((six = (int *)overflow()) == NULL) > > errx(1, "number overflowed"); > > return (eight); > ^ NO BSD style return () > > > > > Branch Prediction > > ----------------- > > > > When a test is done in a critical zone (called often or in a data path) use the ``likely()`` and ``unlikely()`` macros. They are expanded > > as a compiler builtin and allow the developer to indicate if the branch is likely to be taken or not. Example: > > #include > > if (likely(x > 1)) > > do_stuff(); > > You need to stress that likely() and unlikely() should be used sparingly. > Many times the compiler knows better. > > > > > > > Static Variables and Functions > > ------------------------------ > > > > All functions and variables that are local to a file must be declared as ``static`` because it can often help the compiler to do > > some optimizations (such as, inlining the code). > > > > Functions that must be inlined have to be declared as ``static inline`` and can be defined in a .c or a .h file. > > > > Const Attribute > > --------------- > > > > Particular care must be taken with the use of the ``const`` attribute. It should be used as often as possible when a variable is read-only. > > > A couple more things: > no UPPER or CamelCase variable names or functions > > The other thing (and Intel is the worst offender) is to avoid excessive inlining. > For example, I am pretty sure that all rte_ring stuff really should not be inline, > the compiler can do a good job (especially with LTO) even without inlining. > Disagree here too. When you are writing code while is polling from a software ring which is likely to be empty a large proportion of the time, e.g. a control queue, the cycle cost of the function call overhead is far greater than the time it takes the poll the empty ring itself. [Measured using the ring unit tests] /Bruce