Re: pserialize(9) vs. TAILQ

[Dennis Ferguson <dennis.c.ferguson%gmail.com@localhost>]

On 19 Nov, 2014, at 21:00 , Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>   Since most machines don't need any barrier here it would be extremely
>   inefficient to add a membar_consumer() since that would make all machines
>   pay for the idiosyncrasies unique to the DEC Alpha.
> 
> We could invent a membar_datadep_consumer for this case, and make it a
> no-op on everything other than Alpha until another CPU designer
> relaxes the memory ordering.  The intent of the code is clearer with
> the memory barrier, and it emphasizes the need for a paired
> membar_producer elsewhere.

The problem with this is that current kernel code already assumes it
isn't running on a multiprocessor Alpha and omits read barriers where
they aren't required for other machines (which likely explains why
there is no appropriate barrier operation for this currently).  As
a gross measure

   b1$ cd /usr/src/sys/kern
   b1$ grep membar_producer * | wc -l 
         25
   b1$ grep membar_consumer * | wc -l 
          5

and it isn't very hard to find explicit examples.  Here's one from
subr_pcq.c

    void *  
    pcq_get(pcq_t *pcq)
    {
            uint32_t v, nv;
            u_int p, c;
            void *item;
   
            v = pcq->pcq_pc;
            pcq_split(v, &p, &c);  
            if (p == c) {
                    /* Queue is empty: nothing to return. */
                    return NULL;
            }
            item = pcq->pcq_items[c];

and another from subr_ipi.c

    static void  
    ipi_msg_cpu_handler(void *arg __unused)
    {                       
            const struct cpu_info * const ci = curcpu();
            ipi_mbox_t *mbox = &ipi_mboxes[cpu_index(ci)];

            for (u_int i = 0; i < IPI_MSG_MAX; i++) {
                    ipi_msg_t *msg;

                    /* Get the message. */
                    if ((msg = mbox->msg[i]) == NULL) {
                            continue;
                    }
                    mbox->msg[i] = NULL;
        
                    /* Execute the handler. */
                    KASSERT(msg->func);
                    msg->func(msg->arg);

I think the Alpha needs barriers before the last lines.

Adding a new barrier operation for this would only help if there
were some determination to also go back and fix the code already
written to use it, i.e. someone needs to decree that all code
now needs to be written to theoretically run on an Alpha (the
support would still only be theoretical since it is hard to
find hardware which behaves like this to test on).  I haven't
heard anything one way or the other concerning support for MP
Alphas, but the implicit message from the current state of
the code is that an MP Alpha isn't a NetBSD target.

In any case (and omitting arguments about why trying to support
the MP Alpha might not be a good idea), before you add the new
barrier I think there needs to explicitly statement to the effect
that the MP Alpha needs to be included as a NetBSD target platform
so that there is an actual need for that barrier.  Without that I
think this code is correct the way it is; there is certainly
no problem with it that is fixed by adding a membar_consumer().

Dennis Ferguson