Re: [dpdk-dev] [PATCH v7] eal: make lcore_config private

[David Marchand <david.marchand@redhat.com>]

Thanks for working on this.

On Wed, Sep 25, 2019 at 6:10 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The internal structure of lcore_config is no longer be part of
> visible API/ABI. Make it private to EAL.
>
> Rearrange and resize the fields in the structure so it takes
> less memory (and cache footprint).

This patch is missing the release notes update.


>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v7 - add eal_private.h to windows
>
>  lib/librte_eal/common/eal_common_launch.c |  2 ++
>  lib/librte_eal/common/eal_private.h       | 22 +++++++++++++++++++++
>  lib/librte_eal/common/include/rte_lcore.h | 24 -----------------------
>  lib/librte_eal/common/rte_service.c       |  2 ++
>  lib/librte_eal/rte_eal_version.map        |  1 -
>  lib/librte_eal/windows/eal/eal_thread.c   |  1 +
>  6 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_launch.c b/lib/librte_eal/common/eal_common_launch.c
> index fe0ba3f0d617..cf52d717f68e 100644
> --- a/lib/librte_eal/common/eal_common_launch.c
> +++ b/lib/librte_eal/common/eal_common_launch.c
> @@ -15,6 +15,8 @@
>  #include <rte_per_lcore.h>
>  #include <rte_lcore.h>
>
> +#include "eal_private.h"
> +
>  /*
>   * Wait until a lcore finished its job.
>   */
> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> index 798ede553b21..25e80547904f 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -10,6 +10,28 @@
>  #include <stdio.h>
>
>  #include <rte_dev.h>
> +#include <rte_lcore.h>
> +
> +/**
> + * Structure storing internal configuration (per-lcore)
> + */
> +struct lcore_config {
> +       uint32_t core_id;      /**< core number on socket for this lcore */
> +       uint32_t core_index;   /**< relative index, starting from 0 */
> +       uint16_t socket_id;    /**< physical socket id for this lcore */
> +       uint8_t core_role;         /**< role of core eg: OFF, RTE, SERVICE */
> +       uint8_t detected;          /**< true if lcore was detected */
> +       volatile enum rte_lcore_state_t state; /**< lcore state */
> +       rte_cpuset_t cpuset;       /**< cpu set which the lcore affinity to */
> +       pthread_t thread_id;       /**< pthread identifier */
> +       int pipe_master2slave[2];  /**< communication pipe with master */
> +       int pipe_slave2master[2];  /**< communication pipe with master */
> +       lcore_function_t * volatile f;         /**< function to call */
> +       void * volatile arg;       /**< argument of function */
> +       volatile int ret;          /**< return value of function */
> +};
> +
> +extern struct lcore_config lcore_config[RTE_MAX_LCORE];

Everything but cpuset can fit in a cache line.
You could just move the cpuset field at the end of the structure and
change detected to uint8_t.
This gives the following layout:

struct lcore_config {
    pthread_t                  thread_id;            /*     0     8 */
    int                        pipe_master2slave[2]; /*     8     8 */
    int                        pipe_slave2master[2]; /*    16     8 */
    volatile lcore_function_t *  f;                  /*    24     8 */
    volatile void *            arg;                  /*    32     8 */
    volatile int               ret;                  /*    40     4 */
    volatile enum rte_lcore_state_t  state;          /*    44     4 */
    unsigned int               socket_id;            /*    48     4 */
    unsigned int               core_id;              /*    52     4 */
    int                        core_index;           /*    56     4 */
    uint8_t                    detected;             /*    60     1 */
    uint8_t                    core_role;            /*    61     1 */

    /* XXX 2 bytes hole, try to pack */

    /* --- cacheline 1 boundary (64 bytes) --- */
    rte_cpuset_t               cpuset;               /*    64   128 */
    /* --- cacheline 3 boundary (192 bytes) --- */

    /* size: 192, cachelines: 3, members: 13 */
    /* sum members: 190, holes: 1, sum holes: 2 */
};

The resulting structure is only two bytes bigger than your proposal
and does not touch existing integer types (avoiding the risk of some
integer conversion on socket_id for example).


-- 
David Marchand