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

Le 12/08/2020 à 15:46, Nathan Lynch a écrit :

Hi Christophe,

Christophe Leroy [off-list ref] writes:

quoted

quoted

+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.

quoted

quoted

+
+	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.

quoted

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.

quoted

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