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
