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).

Will have a look tomorrow.

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

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.

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.

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

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.

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.