Re: uprobe splat in PREEMP_RT

[Oleg Nesterov <oleg@redhat.com>]

On 04/03, Sebastian Sewior wrote:
>
> On 2025-04-02 16:23:50 [+0200], Oleg Nesterov wrote:
> > OK, it seems we can't understand each other. Probably my fault.
> >
> > So, it think that
> >
> > 	static inline bool __seqprop_preemptible(const seqcount_t *s)
> > 	{
> > 		return false;
> > 	}
> >
> > should return true. Well because it is preemptible just like
> > SEQCOUNT_LOCKNAME(mutex) or, if PREEMPT_RT=y, SEQCOUNT_LOCKNAME(spinlock).
> >
> > Am I wrong?
>
> A seqcount_t has no lock associated with so it is not preemptible.

Well, I still disagree...

> It
> always refers to the lock. This should come from extern so it not only
> disables preemption but also ensures that there can only be one writer.

Yes, but

> The "disabling preemption" is only there to ensure progress is made in
> reasonable time: You should not get preempted in your write section. If
> the writer gets preempted then nothing bad (as in *boom*) will happen
> because you ensured that you have only one writer can enter the section.
> In that scenario the reader will spin on the counter until the writer
> gets back on the CPU and completes the write section and while doing so
> wasting resources. No boom, just wasting resources.

This can lead to deadlock.

Suppose we have a seqcount_t SEQ, and we ensure that it has a single
writer. Say, this SEQ is protected by mutex.

The writer does write_seqcount_begin(&SEQ) on a UP machine, and it is
preeempted by a real-time process which does read_seqcount_begin(&SEQ).
The reader will spin "forever".

> If you make __seqprop_preemptible() return true then
> write_seqcount_begin() for seqcount_t will disable preemption on its
> own. You could now remove all preempt_disable() around its callers. So
> far so good as everyone should have one.

Yes,

> The problem here is that for !RT only seqcount_mutex_t gets preemption
> disabled.

Sure, for !RT spinlock/rwlock disable preemption,

> For RT none of the seqcount_t variants get preemption disabled
> but rely on lock+unlock mechanism to ensure that progress is made.

Yes I know, but seqcount_t doesn't have the associated lock.

> With this change it would also disable preemption for RT and I don't
> know if it is is always the smart thing to do.

I don't know either.

But the current logic doesn't look right to me.

From include/linux/seqlock.h

	SEQCOUNT_LOCKNAME(raw_spinlock, raw_spinlock_t,  false,    raw_spin)
	SEQCOUNT_LOCKNAME(spinlock,     spinlock_t,      __SEQ_RT, spin)
	SEQCOUNT_LOCKNAME(rwlock,       rwlock_t,        __SEQ_RT, read)
	SEQCOUNT_LOCKNAME(mutex,        struct mutex,    true,     mutex)

so for these seqcount's seqprop_preemptible() returns false only if
the associated lock disables preemption. raw_spinlock always does this
spinlock/rwlock depend on PREEMPT_RT.

But seqcount_t doesn't have the associated lock, so I think that
seqprop_preemptible(seqcount_t) should return true.

But OK, I won't insist. At least it seems we more or less understand
each other ;)

Oleg.