Re: [dpdk-dev] [PATCH v1 1/5] eal: add accessor functions for lcore_config

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

On Mon, Apr 8, 2019 at 8:25 PM Stephen Hemminger <stephen@networkplumber.org>
wrote:

> The fields of the internal EAL core configuration are currently
> laid bare as part of the API. This is not good practice and limits
> fixing issues with layout and sizes.
>
> Make new accessor functions for the fields used by current drivers
> and examples. Mark the state and return code functions as experimental
> since these values might change in the future and probably shouldn't
> have been used by non EAL code anyway.
>

You removed the state function, no need to mention it.

Since we already expose the state, exposing the ret code seems fair.
Applications can wait themselves and check the return code the way they
want.

Just want to point that the current test app could do without it, since it
is quite simple, it waits for all, then checks all return codes.



> Most of these functions are not marked as experimental since
> we want applications to convert over asap. The one exception is
> the return_code, which is only used by some tests now and not
> clear that it is even that useful.
>

+1


diff --git a/lib/librte_eal/common/eal_common_lcore.c
> b/lib/librte_eal/common/eal_common_lcore.c
> index 1cbac42286ba..c3cf5a06269d 100644
> --- a/lib/librte_eal/common/eal_common_lcore.c
> +++ b/lib/librte_eal/common/eal_common_lcore.c
> @@ -16,6 +16,45 @@
>  #include "eal_private.h"
>  #include "eal_thread.h"
>
> +int rte_lcore_index(int lcore_id)
> +{
> +       if (unlikely(lcore_id >= RTE_MAX_LCORE))
> +               return -1;
> +
> +       if (lcore_id < 0)
> +               lcore_id = (int)rte_lcore_id();
> +
> +       return lcore_config[lcore_id].core_index;
> +}
> +
> +int rte_lcore_to_cpu_id(int lcore_id)
> +{
> +       if (unlikely(lcore_id >= RTE_MAX_LCORE))
> +               return -1;
> +
> +       if (lcore_id < 0)
> +               lcore_id = (int)rte_lcore_id();
> +
> +       return lcore_config[lcore_id].core_id;
> +}
> +
> +rte_cpuset_t rte_lcore_cpuset(unsigned int lcore_id)
> +{
> +       return lcore_config[lcore_id].cpuset;
> +}
> +
> +unsigned
>

Ah, checkpatch does not complain for me, I would have expected a warning on
"unsigned int".
Can you fix it so that it matches the declaration from the header ?

+rte_lcore_to_socket_id(unsigned int lcore_id)
> +{
> +       return lcore_config[lcore_id].socket_id;
> +}
> +
> +int
> +rte_lcore_return_code(unsigned int lcore_id)
> +{
> +       return lcore_config[lcore_id].ret;
> +}
> +
>  static int
>  socket_id_cmp(const void *a, const void *b)
>  {



The rest looks fine.
Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand