Set the refcnt to zero for new memory zones.

[bosilca]

Thanks @ymeur for the report and the proposed fix.

Fixes #12892.

[devreal]

I'm ok with this patch but maybe we should attribute the author of #12892 in the commit? (sorry, can't tag the name here)

[bosilca]

It is. What exactly do you mean ?

[devreal]

I meant a Signed-off-by or Co-authored-by line but we don't have an email address i guess

[MamziB]

@bosilca @devreal @janjust
I took a look at the code today. When we attach we increment the ref count and when we detach, we decrease it. Therefore, we only need to set the ref count to 0 in the osc init. This PR is not needed, unless there is a specific benchmark that fails/has mem leak with this design.

[bosilca]

@MamziB the original issue reference both the need for this fix and the rationale. This looks correct to me, we need to set the refcount to zero when we create a new memory zone, and this is exactly what we do here (we make room into the existing allocations array, and as a result this allocation will inherit the refcount of the previous one).

[MamziB]

@bosilca Yes, I am aware of that issue.

We have a maximum of OMPI_OSC_UCX_ATTACH_MAX dynamic windows that can be created, and the local_dynamic_win_info array of this size keeps track of any window that is allocated.

During initialization, component_select ensures that all reference counts are set to zero. As a result, we no longer need to manually reset them. When a new window is created, we can be certain that the assigned slot among the OMPI_OSC_UCX_ATTACH_MAX slots is available—either because it was previously detached or has never been used. In both cases, the reference count must already be zero.

[bosilca]

That's a different story. The two memmove are creating room in the local_dynamic_win_info array for the new zone to be inserted. In a location that was occupied, therefore with a non zero refcount. If we don't reset the refcount of the insert_index location, this new memory zone will inherit the refcount of the prior zone.

[MamziB]

@bosilca Thanks for the explanation, I see your point now and this PR is indeed needed.