Re: + ipc-convert-ipcs_idr-to-xarray-update.patch added to -mm tree

[Manfred Spraul <manfred@colorfullife.com>]

Hi Matthew,

Due to lack of time, I didn't follow up immediately, but I haven't 
forgotten the patch series:

On 4/24/20 3:47 AM, Andrew Morton wrote:
> The patch titled
>       Subject: ipc-convert-ipcs_idr-to-xarray-update

[...]

> -		xas_lock(&xas);
> +		xa_lock(&ids->ipcs);
>   
> -		min_idx = ids->next_idx;
> -		new->seq = ids->seq;
> +		err = __xa_alloc_cyclic(&ids->ipcs, &idx, NULL,
> +				XA_LIMIT(0, max_idx), &ids->next_idx,
> +				GFP_KERNEL);
> +		if (err == 1) {
> +			ids->seq++;
> +			if (ids->seq >= ipcid_seq_max())
> +				ids->seq = 0;
> +		}
> +
> +		if (err >= 0) {
> +			new->seq = ids->seq;
> +			new->id = (new->seq << ipcmni_seq_shift()) + idx;
> +			/* xa_store contains a write barrier */
> +			__xa_store(&ids->ipcs, idx, new, GFP_KERNEL);
> +		}

The GFP_KERNEL is really ugly.

When I see "GFP_KERNEL", it means for me that the called function might 
sleep.
When I see "GFP_ATOMIC", it means for me that the called function may 
fail, and that the caller must implement a retry algorithm.

I think the GFP_KERNEL here means nothing, but I'm not sure.

Would it be possible to make __xa_store(,,,0) an officially supported 
feature?
- The function will fail if there was no previous xa_alloc for exactly 
the to be stored entry (or an xa_insert, or an xa_store(,,,GFP_KERNEL), 
etc.)
- The function will not allocate any memory.
- The function will not fail due to out of memory.
- The function will not drop the xa_lock()

Then the usage would be clear:
    xa_alloc(,GFP_KERNEL)
    acquire_all_locks()
    __xa_store(,,,0)
    drop_all_locks()


The kerneldoc is for me not clear enough:

> **
>  * __xa_store() - Store this entry in the XArray.
>  * @xa: XArray.
>  * @index: Index into array.
>  * @entry: New entry.
>  * @gfp: Memory allocation flags.
>  *
>  * You must already be holding the xa_lock when calling this function.
>  * It will drop the lock if needed to allocate memory, and then reacquire
>  * it afterwards.
Is it guaranteed the dropping/acquiring the lock will never happen after 
storing the entry into the array?
If the xarray code first stores the entry, and then allocates memory for 
tree balancing or similar, then it would break the ipc/*.c

--

     Manfred