Hi All,
From: dev [mailto:dev-boun...@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 at intel.com>
