epoch support for taskqueues

[glebius]

Summary

Add optional epoch to struct task and struct gtask. When processing
a taskqueue and a task has associated epoch, then enter for duration
of the task. If consecutive tasks belong to the same epoch, batch them.

Revert manual NET_EPOCH_ENTER/EXIT from network drivers that
process packets in a taskqueue context.

https://github.com/glebius/FreeBSD/commits/taskq-epoch

[bz]

In general I like the idea. Still need to touch network drivers but changing a single macro seems way better to me. It's also easily grep-able.

I am not sure if this change backs out all drivers which have since been changed (by others possibly).

sys/sys/epoch.h

108

I wonder if it would make sense to have a GROUPTASK_INIT2(..) as well and not encode the GTASK_INIT2 flags here but in gtaskqueue.h?

[melifaro]

sys/kern/subr_gtaskqueue.c

367

Why do we want to potentially widen an epoch here/in taskq from day 1? The current usecase seem to be the driver submitting the packet(s) via tasq, which should be batched for heavy workloads already.

[glebius]

sys/kern/subr_gtaskqueue.c

367

There is not good reason not to batch it as long as we just need to make one comparison to achieve that.

sys/sys/epoch.h

108

Not sure. GROUPTASK_INIT() creates one more level of obscurity, macro over macro for no good reason. I'd prefer it didn't exist at all. But if you insist, we can do that.

[bz]

sys/sys/epoch.h

108

I agree that the less macros the better; I was just worried that pulling gtaskqueue.h details such as TASK_SKIP_WAKEUP into epoch.h is a bit messy.
I have no hard feelings about it, so either way I am ok.

[• hselasky]

Will have a look tomorrow.

[glebius]

• Add a temporary compatibility hack to ease transition to new ABI
• Bump kernel version.

[• hselasky]

Hi,

In the kernel there are 403 instances of TASK_INIT(). In this patch I only see NET_TASK_INIT() used 5 times. Does this justify the increased memory usage of "struct task".

grep -r TASK_INIT\( sys/ | grep -v GTASK | wc -l

403

Further, if EPOCH(9) gets special treatment by taskqueues, why are not mutexes then given special treatment, similar to callouts, to support "atomic" stopping of tasks w/o need for drain, similar to what callout_stop() does?

--HPS

[glebius]

In D23408#513926, @hselasky wrote:

In the kernel there are 403 instances of TASK_INIT(). In this patch I only see NET_TASK_INIT() used 5 times. Does this justify the increased memory usage of "struct task".
grep -r TASK_INIT\( sys/ | grep -v GTASK | wc -l

403

Seriously? You are against one pointer size increase for struct task, but at the same time you propose code increase for several dozens of drivers.

Further, if EPOCH(9) gets special treatment by taskqueues, why are not mutexes then given special treatment, similar to callouts, to support "atomic" stopping of tasks w/o need for drain, similar to what callout_stop() does?

Cause epochs aren't mutexes. Prolonged mutex sections affect concurrency and prolonged epoch sections doesn't. Sorry for saying trivial things and repeating myself.

[• hselasky]

In D23408#514121, @glebius wrote:

In D23408#513926, @hselasky wrote:

In the kernel there are 403 instances of TASK_INIT(). In this patch I only see NET_TASK_INIT() used 5 times. Does this justify the increased memory usage of "struct task".
grep -r TASK_INIT\( sys/ | grep -v GTASK | wc -l

403

Seriously? You are against one pointer size increase for struct task, but at the same time you propose code increase for several dozens of drivers.

Further, if EPOCH(9) gets special treatment by taskqueues, why are not mutexes then given special treatment, similar to callouts, to support "atomic" stopping of tasks w/o need for drain, similar to what callout_stop() does?

Cause epochs aren't mutexes. Prolonged mutex sections affect concurrency and prolonged epoch sections doesn't. Sorry for saying trivial things and repeating myself.

I think we discussed this yesterday, so I'm not replying to this one.

sys/sys/gtaskqueue.h

91–96

There is a whitespace after epoch, before )

Should this function have a better name?

GTASK_INIT_EPOCH() ?

sys/sys/taskqueue.h

125–130

Ditto - TASK_INIT_EPOCH() ??

[• hselasky]

Hi,

I found 18 places where this function must be used, while this review currently only address 4?

% svn diff | grep NET_TASK_INIT
+	NET_TASK_INIT(&rx_ring->enqueue_task, 0, al_eth_rx_recv_work, rx_ring);
+	NET_TASK_INIT(&sc->alc_int_task, 0, alc_int_task, sc);
+	NET_TASK_INIT(&sc->ale_int_task, 0, ale_int_task, sc);
+	NET_TASK_INIT(&sc->bge_intr_task, 0, bge_intr_task, sc);
+        NET_TASK_INIT(&fp->tq_task, 0, bxe_handle_fp_tq, fp);
+	NET_TASK_INIT(&sc->sc_intr_task, 0, cas_intr_task, sc);
+		NET_TASK_INIT(&queue->cleanup_task, 0, ena_cleanup, queue);
+			NET_TASK_INIT(&pq->task, 0, ptnet_rx_task, pq);
+	NET_TASK_INIT(&sc->nfe_int_task, 0, nfe_int_task, sc);
+                NET_TASK_INIT(&fp->fp_task, 0, qla_fp_taskqueue, fp);
+	NET_TASK_INIT(&sc->rl_inttask, 0, re_int_task, sc);
+	NET_TASK_INIT(&sc->rx_done_task, 0, rt_rx_done_task, sc);
+	NET_TASK_INIT(&sc->smc_rx, SMC_RX_PRIORITY, smc_task_rx, ifp);
+	NET_TASK_INIT(&rxq->vtnrx_intrtask, 0, vtnet_rxq_tq_intr, rxq);
+	NET_TASK_INIT(&cq->cmp_task, 0, nicvf_cmp_task, cq);
+	NET_TASK_INIT(&qs->qs_err_task, 0, nicvf_qs_err_task, nic);
+	NET_TASK_INIT(&sc->vr_inttask, 0, vr_int_task, sc);
+	NET_TASK_INIT(&sc->xl_task, 0, xl_rxeof_task, sc);
% svn diff | grep NET_TASK_INIT | wc -l
      18

[• hselasky]

sys/kern/subr_taskqueue.c

471

There is another way to do this, which is less intrusive to "struct task".

[• hselasky]

sys/kern/subr_taskqueue.c

475

There is no timeout on this loop, which is dangerous!

478

net_epoch_preempt may be NULL ....

[• hselasky]

Adding 14 more places where NET_TASK_INIT() should be used.

[• hselasky]

Added simple timeout logic for how long the EPOCH should be held. Typically no longer than a tick.

[glebius]

Hans, first you start commit war in subversion and then you "commandeer" my revision. We will not make any progress if you "help" like this.

I did break some drivers one week ago. But the reason they are not fixed yet is you, not me. You are basically putting sticks into my wheels with your commits, with overtaking my reviews, with recruiting me to explain the same things to you for n-th time.

[gallatin]

sys/sys/gtaskqueue.h

91–96

I agree on both points

[jeff]

sys/sys/_task.h

53

on 64 bit platforms there is a 32bit pad here that could be used for flags to specify the epoch rather than a pointer. This would prevent structure size changes.

69

Another way to do this entirely would be to make a network specific taskqueue rather than a network specific task. The taskqueue system already supports specialization.

[glebius]

Since this revision was commandeered, I created a new one: https://reviews.freebsd.org/D23518

I picked up extra driver changes from this one. Thanks, Hans!

I didn't pickup the tick restriction in the cycle. The cycle is finite, I don't see any good reason to limit it unless there is experimental proof of such need. We were talking about limiting to a tick the cycle in the interrupt code. That one is potentially infinite, and there it makes sense. However, with a benchmark first.

I didn't pickup macro renaming. First, it is inconsistent - TASK_INIT2 was renamed, but GTASK_INIT2 was not. Second, the ugly named macro isn't public, so I don't see good reason to rename it. However, I'm not strong in this opinion, I'm open to renaming both TASK_INIT2 and GTASK_INIT2, if more people vote for that.

[• hselasky]

In D23408#516172, @glebius wrote:

Since this revision was commandeered, I created a new one: https://reviews.freebsd.org/D23518

I picked up extra driver changes from this one. Thanks, Hans!

I didn't pickup the tick restriction in the cycle. The cycle is finite, I don't see any good reason to limit it unless there is experimental proof of such need. We were talking about limiting to a tick the cycle in the interrupt code. That one is potentially infinite, and there it makes sense. However, with a benchmark first.

I didn't pickup macro renaming. First, it is inconsistent - TASK_INIT2 was renamed, but GTASK_INIT2 was not. Second, the ugly named macro isn't public, so I don't see good reason to rename it. However, I'm not strong in this opinion, I'm open to renaming both TASK_INIT2 and GTASK_INIT2, if more people vote for that.

You're welcome Gleb. I don't think there is a limit in differential how many times a commit may be commandeered.
I see you got the most important changes, the NET_TASK_INIT() changes. I'll have a closer look later today or tomorrow.

[glebius]

Was abandoned in favor of D23518