Re: global barrier & atomics in signal handlers (Re: Atomic operations within spinlocks)

Hi,

On 2020-06-16 14:59:19 -0400, Robert Haas wrote:
> On Mon, Jun 15, 2020 at 9:37 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > What do you think about 0002?
> >
> > With regard to the cost of the expensive test in 0003, I'm somewhat
> > inclined to add that to the buildfarm for a few days and see how it
> > actually affects the few bf animals without atomics. We can rip it out
> > after we got some additional coverage (or leave it in if it turns out to
> > be cheap enough in comparison).
>
> I looked over these patches briefly today. I don't have any objection
> to 0001 or 0002.

Cool. I was mainly interested in those for now.

> I think 0003 looks a little strange: it seems to be
> testing things that might be implementation details of other things,
> and I'm not sure that's really correct. In particular:

My main motivation was to have something that runs more often than than
the embeded test in s_lock.c's that nobody ever runs (they wouldn't even
pass with disabled spinlocks, as S_LOCK_FREE isn't implemented).

> + /* and that "contended" acquisition works */
> + s_lock(&struct_w_lock.lock, "testfile", 17, "testfunc");
> + S_UNLOCK(&struct_w_lock.lock);
>
> I didn't think we had formally promised that s_lock() is actually
> defined or working on all platforms.

Hm? Isn't s_lock the, as its comment says, "platform-independent portion
of waiting for a spinlock."? I also don't think we need to purely
follow external APIs in internal tests.

> More generally, I don't think it's entirely clear what all of these
> tests are testing. Like, I can see that data_before and data_after are
> intended to test that the lock actually fits in the space allowed for
> it, but at the same time, I think empty implementations of all of
> these functions would pass regression, as would many horribly or
> subtly buggy implementations.

Sure, there's a lot that'd pass. But it's more than we had before. It
did catch a bug much quicker than I'd have otherwise found it, FWIW.

I don't think an empty implementation would pass btw, as long as TAS is
defined.

> So I feel like the intent of these tests isn't entirely clear, and
> should probably be explained better, at a minimum -- and perhaps we
> should think harder about what a good testing framework would look
> like.

Yea, we could use something better. But I don't see that happening
quickly, and having something seems better than nothing.

> I would rather have tests that either pass or fail and report a result
> explicitly, rather than tests that rely on hangs or crashes.

That seems quite hard to achieve. I really just wanted to have something
I can do some very basic tests to catch issues quicker.

The atomics tests found numerous issues btw, despite also not testing
concurrency.

I think we generally have way too few of such trivial tests. They can
find plenty "real world" issues, but more importantly make it much
quicker to iterate when working on some piece of code.

> I don't have any real complaints about the functionality of 0004 on a
> quick read-through, but I'm again a bit skeptical of the tests. Not as
> much as with 0003, though.

Without the tests I couldn't even reproduce a deadlock due to the
nesting. So they imo are pretty essential?

Greetings,

Andres Freund