From mboxrd@z Thu Jan  1 00:00:00 1970
From: Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH v4 04/17] eal: add support parsing socket_id
 from cpuset
Date: Mon, 09 Feb 2015 18:16:53 +0100
Message-ID: <54D8EB85.40809@6wind.com>
References: <1422491072-5114-1-git-send-email-cunming.liang@intel.com>
 <1422842559-13617-1-git-send-email-cunming.liang@intel.com>
 <1422842559-13617-5-git-send-email-cunming.liang@intel.com>
 <54D7C042.6060104@6wind.com>
 <D0158A423229094DA7ABF71CF2FA0DA3118D7D72@shsmsx102.ccr.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
To: "Liang, Cunming" <cunming.liang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, "dev-VfR2kkLFssw@public.gmane.org" <dev-VfR2kkLFssw@public.gmane.org>
Return-path: <dev-bounces-VfR2kkLFssw@public.gmane.org>
In-Reply-To: <D0158A423229094DA7ABF71CF2FA0DA3118D7D72-E2R4CRU6q/6iAffOGbnezLfspsVTdybXVpNB7YpNyf8@public.gmane.org>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev-VfR2kkLFssw@public.gmane.org>
List-Help: <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=subscribe>
Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org
Sender: "dev" <dev-bounces-VfR2kkLFssw@public.gmane.org>

Hi,

On 02/09/2015 01:26 PM, Liang, Cunming wrote:
>>> @@ -50,4 +54,52 @@ __attribute__((noreturn)) void *eal_thread_loop(void
>> *arg);
>>>   */
>>>  void eal_thread_init_master(unsigned lcore_id);
>>>
>>> +/**
>>> + * Get the NUMA socket id from cpu id.
>>> + * This function is private to EAL.
>>> + *
>>> + * @param cpu_id
>>> + *   The logical process id.
>>> + * @return
>>> + *   socket_id or SOCKET_ID_ANY
>>> + */
>>> +unsigned eal_cpu_socket_id(unsigned cpu_id);
>>
>> Wouldn't it be better to rename the existing function cpu_socket_id()
>> in eal_cpu_socket_id() and export it in eal_thread.h?
>>
>> In case of bsd where cpu_socket_id() is implemented using a #define,
>> a new function should be created returning 0.
> [LCM] In eal_lcore.c, the cpu_socket_id()/cpu_core_id() defined as static and only used in rte_eal_cpu_init().
> I suppose the purpose of origin design is to make the sysfs parsing only visible in the file.
> No matter remove the 'static' prefix of cpu_core_id() or add a new wrap eal_cpu_socket_id(), it results in a new extern EAL API.
> So I prefer not change the visibility of the origin static function but have one as extern interface.

Yes, but I don't see what is the advantage of using a wrapper.
If there is no advantage, I think the one with the less code is
better.



>>> +static inline int
>>> +eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
>>> +{
>>> +	unsigned cpu = 0;
>>> +	int socket_id = SOCKET_ID_ANY;
>>> +	int sid;
>>> +
>>> +	if (cpusetp == NULL)
>>> +		return SOCKET_ID_ANY;
>>> +
>>> +	do {
>>> +		if (!CPU_ISSET(cpu, cpusetp))
>>> +			continue;
>>> +
>>> +		if (socket_id == SOCKET_ID_ANY)
>>> +			socket_id = eal_cpu_socket_id(cpu);
>>> +
>>> +		sid = eal_cpu_socket_id(cpu);
>>> +		if (socket_id != sid) {
>>> +			socket_id = SOCKET_ID_ANY;
>>> +			break;
>>> +		}
>>> +
>>> +	} while (++cpu < RTE_MAX_LCORE);
>>> +
>>> +	return socket_id;
>>> +}
>>
>>
>> I don't think this function should be inlined.
>>
>> As this function is not used, it could be interesting for reviewers
>> to understand when
> [LCM] It's used in eal_thread_set_affinity() of eal_thread.c.

As it's not visible in the patch, could you add an explanation in
the commit log?