Re: [dpdk-dev] [PATCH v2] examples/skeleton-cat: PQoS CAT and CDP, example of libpqos usage

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

Hi All,

From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wojciech Andralojc
> Subject: [dpdk-dev] [PATCH v2] examples/skeleton-cat: PQoS CAT and CDP, example of libpqos
> usage

Some minor comments/suggested-fixes in-line.
I've snipped away lots of code, below only that relevant to my comments.


> diff --git a/doc/guides/sample_app_ug/skeleton-cat.rst
> b/doc/guides/sample_app_ug/skeleton-cat.rst
> +CAT and CDP features allow management of the CPU's last level cache.
> +CAT introduces classes of service (COS) that are essentially bitmasks.

CDP is not explained anywhere - add (Cache Data Partitioning) after it here,
or write it and abbreviate as COS is.



> +Running the Application
> +-----------------------
> +
> +To run the example in a ``linuxapp`` environment and enable CAT on cpus 0-2:
> +
> +.. code-block:: console
> +
> +    ./build/basicfwd-cat -c 2 -n 4 -- --l3ca=0x3@(0-2)

This doesn't work on my machine:
 - Bash complains about syntax error near (
   Quotes around it fixes    --l3ca=0x3@"(0-2)"
 - the -c 2 will enable core represented by bitmask 0x2, so only the 2nd core.
   Is this intended?


> +or to enable CAT and CDP on cpus 1,3:
> +
> +.. code-block:: console
> +
> +    ./build/basicfwd-cat -c 2 -n 4 -- --l3ca=(0x00C00,0x00300)@(1,3)

Add a note here that when running on a maschine that doesn't support CDP, the
"CDP requested but not supported" and "PQOS: L3CA init failed!" errors 
will show up.


<snip very useful PQOS sections>


> +The ``main()`` also allocates a mempool to hold the mbufs (Message Buffers)
> +used by the application:

>From here until the end of the RST doc is duplicated from the basicfwd skeleton
app: remove all the duplicated content.



> +/*
> + * Initializes a given port using global settings and with the RX buffers
> + * coming from the mbuf_pool passed as a parameter.
> + */
> +static inline int
> +port_init(uint8_t port, struct rte_mempool *mbuf_pool)
> +{

Multi-line comment duplicated from basicfwd: can be removed.


> +/*
> + * The lcore main. This is the main thread that does the work, reading from
> + * an input port and writing to an output port.
> + */
> +static __attribute__((noreturn)) void
> +lcore_main(void)
> +{

Same.

> +/*
> + * The main function, which does initialization and calls the per-lcore
> + * functions.
> + */
> +int
> +main(int argc, char *argv[])
> +{

and same again :)


> +	/* Initialize the Environment Abstraction Layer (EAL). */
> +	int ret = rte_eal_init(argc, argv);
> +	if (ret < 0)
> +		rte_exit(EXIT_FAILURE, "Error with EAL initialization\n");
> +
> +	argc -= ret;
> +	argv += ret;
> +
> +	ret = cat_init(argc, argv);
> +	if (ret < 0)
> +		rte_exit(EXIT_FAILURE, "PQOS: L3CA init failed!\n");

Add a short comment above cat_init() stating it initializes PQOS/CAT,
that the API is similar to rte_eal_init() and to refer to the
sample-app documentation for details.


Two checkpatch issues:

WARNING: Block comments use a trailing */ on a separate line
#1231: FILE: examples/skeleton-cat/cat.c:421:
+                                * then default one (#0) */

WARNING: Block comments use a trailing */ on a separate line
#1360: FILE: examples/skeleton-cat/cat.c:550:
+                        * to be used for that group on that socket */



Apart from these minor issues;
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>