[PATCH v2] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal

[Christophe Leroy]

Le 12/08/2020 à 15:46, Nathan Lynch a écrit :
> Hi Christophe,
> 
> Christophe Leroy <christophe.leroy at csgroup.eu> writes:
>>> +static inline struct drmem_lmb *drmem_lmb_next(struct drmem_lmb *lmb)
>>> +{
>>> +	const unsigned int resched_interval = 20;
>>> +
>>> +	BUG_ON(lmb < drmem_info->lmbs);
>>> +	BUG_ON(lmb >= drmem_info->lmbs + drmem_info->n_lmbs);
>>
>> BUG_ON() shall be avoided unless absolutely necessary.
>> Wouldn't WARN_ON() together with an early return be enough ?
> 
> Not sure what a sensible early return behavior would be. If the iterator
> doesn't advance the pointer the behavior will be a hang.

I was thinking about returning lmb++ without checking the cond_resched 
stuff.

> 
> BUG_ON a bounds-check failure is appropriate here; many users of this
> API will corrupt memory otherwise.

It looks really strange to me to do bounds-checks in an iterator like 
this. Should be checked before entering the loop.

> 
>>> +
>>> +	if ((lmb - drmem_info->lmbs) % resched_interval == 0)
>>> +		cond_resched();
>>
>> Do you need something that precise ? Can't you use 16 or 32 and use a
>> logical AND instead of a MODULO ?
> 
> Eh if you're executing in this code you've already lost with respect to
> performance considerations at this level, see the discussion on v1. I'll
> use 16 since I'm going to reroll the patch though.
> 
>> And what garanties that lmb is always an element of a table based at
>> drmem_info->lmbs ?
> 
> Well, that's its only intended use right now. There should not be any
> other arrays of drmem_lmb objects, and I hope we don't gain any.
> 
> 
>> What about:
>>
>> static inline struct drmem_lmb *drmem_lmb_next(struct drmem_lmb *lmb,
>> struct drmem_lmb *start)
>> {
>> 	const unsigned int resched_interval = 16;
>>
>> 	if ((++lmb - start) & resched_interval == 0)
>                             ^^^
> Did you mean '%' here? The bitwise AND doesn't do what I want.

I meant '& (resched_interval - 1)' indeed. But yes we can leave the %, 
GCC will change a % 16 to an & 15.

> 
> Otherwise, making drmem_lmb_next() more general by adding a 'start'
> argument could ease refactoring to come, so I'll do that.

Yes, the main idea is to avoid the access to a global variable in such 
an helper.

> 

Christophe