> On 3 Dec 2018, at 19:10, Doug Ledford <dledford@xxxxxxxxxx> wrote:
>
> On Mon, 2018-12-03 at 17:14 +0100, Håkon Bugge wrote:
>> Thanks for the review, Doug!
>
> You're welcome.
>
>> I do not like this series either. Let me suggest; 1) I clean the
>> commits up and 2), I go back to re-evaluate the use of Port GUID
>> instead of Node GUID.
>>
>> The reason for 2) is that I do not think the ibacm address file will change for most cases, which I presume is a single HCA with one of two ports, and only one address assigned per port.
>
> I wouldn't assume one address per port. Every P_Key on a port creates
> additional addresses to be stored.
Yes, but in another endpoint, as endpoints are designated by Node GUID, Port, and P_Key.
Having looked into proper read/write locking, it will be hairy, because ibacm returns pointers to the (now) dynamic addr_info array. For example, in acm_addr_lookup(), one could add locking such as:
ep = container_of(endpoint, struct acmc_ep, endpoint);
pthread_rwlock_rdlock(&ep->addr_info_lock);
for (i = 0; i < ep->nmbr_ep_addrs; i++)
if (!acm_addr_cmp(&ep->addr_info[i].addr, addr, addr_type)) {
addr = &ep->addr_info[i].addr;
break;
}
pthread_rwlock_unlock(&ep->addr_info_lock);
return addr;
But this function returns a pointer to what the rw lock protected, so after the unlock, the address area pointed to by the return value may have been freed and resurrected. So, the lock must be held until the returned pointer "has been fully consumed".
I will look into 2) above instead.
>> After all, the address file doesn't contain any GUIDs. If the address file doesn't change, my take is to abandon this series and use Port GUIDs instead. The fix, in case of incompatibility, would simply be to delete the address file before starting ibacm.
>>
>> And me confused, since ibacm creates the address file if it is not
>> there, why isn't created _every_ time ibacm is started? And if so, why
>> is it needed at all?
>
> For performance reasons I think. Doesn't it also store remote addresses
> that it has learned over time? In a 10,000 node cluster, being able to
> prime your machine name -> address mapping table instead of having to
> relearn it on reboot is a big time saver. But I could be off, it's been
> a while since I looked into ibacm.
By default, there is no storing of addresses. It was in legacy versions, but now we have the following option in the ibacm_opts.cfg file:
# support_ips_in_addr_cfg:
# If 1 continue to read IP addresses from ibacm_addr.cfg
# Default is 0 "no"
# support_ips_in_addr_cfg 0
I will see if I can have an option to select Node vs. Port GUID. This way, the backwards compatibility issues will be solved.
Thxs, Håkon
