Re: [PATCH v12 3/3] ipc: Do cyclic id allocation with ipcmni_extend mode

[Manfred Spraul]

This is a multi-part message in MIME format.
Hi Waiman,

On 2/28/19 7:47 PM, Waiman Long wrote:
> For ipcmni_extend mode, the sequence number space is only 7 bits. So
> the chance of id reuse is relatively high compared with the non-extended
> mode.
>
> To alleviate this id reuse problem, the id allocation will be done
> cyclically to cycle through all the 24-bit id space before wrapping
> around when in ipcmni_extend mode. This may cause the use of more memory
> in term of the number of xa_nodes allocated as well as potentially more
> cachelines used as the xa_nodes may be spread more sparsely in this case.
>
> There is probably a slight memory and performance cost in doing cyclic
> id allocation. For applications that really need more than 32k unique IPC
> identifiers, this is a small price to pay to avoid the id reuse problem.

Have you measured it?

I have observed -3% for semop() for a 4 level radix tree compared to a 
1-level radix tree, and I'm a bit reluctant to accept that.
Especially as the percentage will increase if the syscall overhead goes 
down again (-> less spectre impact).

[...]

> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -221,7 +221,12 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
>   	 */
>   
>   	if (next_id < 0) { /* !CHECKPOINT_RESTORE or next_id is unset */
> -		idx = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
> +		if (ipc_mni_extended)
> +			idx = idr_alloc_cyclic(&ids->ipcs_idr, new, 0, ipc_mni,
> +						GFP_NOWAIT);
> +		else
> +			idx = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
> +
>   		if ((idx <= ids->last_idx) && (++ids->seq > IPCID_SEQ_MAX))
>   			ids->seq = 0;

I don't like it that there are two different codepaths.

Attached is a different proposal:

Always use cyclic allocation, with some logic to minimize the additional 
radix tree levels.

What do you think?

--

ÂÂÂ Manfred