On 19 Nov, 2014, at 21:00 , Taylor R Campbell <campbell+netbsd-tech-k...@mumble.net> 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