On Sat, Aug 12, 2017 at 09:50 +0300, Atanas Vladimirov wrote:
> Hi,
>
> My home server (-current) freeze with kernel panic and I have to reboot it
> because no keyboard or IPMI working.
>
> panic: kernel diagnostic assertion "TAILQ_EMPTY(&cl->cl_actc)" failed: file
> "/usr/src/sys/net/hfsc.c", line 767
> Stopped at db_enter+0x5: popq %rbp
> TID PID UID PRFLAGS PFLAGS CPU COMMAND
> *395466 8691 0 0x2 0 0 pfctl
>
> I managed to take a screenshot (send as attachment).
>
> Recently a add the following to pf.conf to test flows:
>
> ....
> ### Queueing
>
> queue hfs-fq on $ExtIf flows 1024 bandwidth 40M max 45M quantum 400 qlimit
> 1000 default
> ....
>
Hi,
I've finally managed to figure out what's going on.
Turns out that when hfsc_deq_begin would fail to dequeue from the
fq_codel queue (that's normal) it would mark the class as passive.
Except it won't. The correct procedure is to do what _deq_commit
does which *effectively* boils down to this one line diff:
diff --git sys/net/hfsc.c sys/net/hfsc.c
index 3c5b6f6ef78..2b349e3cb52 100644
--- sys/net/hfsc.c
+++ sys/net/hfsc.c
@@ -883,10 +883,11 @@ hfsc_deq_begin(struct ifqueue *ifq, void **cookiep)
m = hfsc_class_deq_begin(cl, &free_ml);
ifq_mfreeml(ifq, &free_ml);
if (m == NULL) {
/* the class becomes passive */
+ hfsc_update_vf(cl, 0, cur_time);
hfsc_set_passive(hif, cl);
return (NULL);
}
hif->hif_microtime = cur_time;
What hfsc_update_vf does in this case (when queue is empty) is
it removes the class from it's parents active child class list.
Which is precisely what that KASSERT asserts during purging:
there must be no active child classes if the queue is empty.
However this doesn't sit very well with me since it's not 100%
complete and I'd like to factor out the whole procedure (not
just what it boils down to) out of hfsc_deq_commit into a
separate function hfsc_update_sc (sc for the service curve).
I believe this makes it more obvious what's going on and at
the same time I'm adding (or rather reverting) this bit:
- m0 = hfsc_class_deq_begin(cl, &free_ml);
- ifq_mfreeml(ifq, &free_ml);
+ KASSERT(cl->cl_qops == pfq_hfsc_ops);
+ m0 = MBUF_LIST_FIRST(&cl->cl_q.q);
This is done for clarity reasons: the realtime curve can't
be used with fq_codel since it needs to know what packet will
come next and in case of fq_codel we don't know that before
we attempt to dequeue. Initially I decided to use the newly
established API here as well, but now I think it's largely
detrimental to the overall understanding of what's going on
so I'm back-pedalling on it (plus it saves me passing the ifq
pointer for the freelist that we don't need -- it was there
only to satisfy the API).
OK?
diff --git sys/net/hfsc.c sys/net/hfsc.c
index 3c5b6f6ef78..12504267dc5 100644
--- sys/net/hfsc.c
+++ sys/net/hfsc.c
@@ -206,10 +206,11 @@ int hfsc_class_destroy(struct
hfsc_if *,
struct hfsc_class *hfsc_nextclass(struct hfsc_class *);
void hfsc_cl_purge(struct hfsc_if *, struct hfsc_class *,
struct mbuf_list *);
+void hfsc_update_sc(struct hfsc_if *, struct hfsc_class *, int);
void hfsc_deferred(void *);
void hfsc_update_cfmin(struct hfsc_class *);
void hfsc_set_active(struct hfsc_if *, struct hfsc_class *, int);
void hfsc_set_passive(struct hfsc_if *, struct hfsc_class *);
void hfsc_init_ed(struct hfsc_if *, struct hfsc_class *, int);
@@ -882,12 +883,11 @@ hfsc_deq_begin(struct ifqueue *ifq, void **cookiep)
}
m = hfsc_class_deq_begin(cl, &free_ml);
ifq_mfreeml(ifq, &free_ml);
if (m == NULL) {
- /* the class becomes passive */
- hfsc_set_passive(hif, cl);
+ hfsc_update_sc(hif, cl, 0);
return (NULL);
}
hif->hif_microtime = cur_time;
*cookiep = cl;
@@ -895,39 +895,45 @@ hfsc_deq_begin(struct ifqueue *ifq, void **cookiep)
}
void
hfsc_deq_commit(struct ifqueue *ifq, struct mbuf *m, void *cookie)
{
- struct mbuf_list free_ml = MBUF_LIST_INITIALIZER();
struct hfsc_if *hif = ifq->ifq_q;
struct hfsc_class *cl = cookie;
- struct mbuf *m0;
+
+ hfsc_class_deq_commit(cl, m);
+ hfsc_update_sc(hif, cl, m->m_pkthdr.len);
+
+ PKTCNTR_INC(&cl->cl_stats.xmit_cnt, m->m_pkthdr.len);
+}
+
+void
+hfsc_update_sc(struct hfsc_if *hif, struct hfsc_class *cl, int len)
+{
int next_len, realtime = 0;
u_int64_t cur_time = hif->hif_microtime;
/* check if the class was scheduled by real-time criteria */
if (cl->cl_rsc != NULL)
realtime = (cl->cl_e <= cur_time);
- hfsc_class_deq_commit(cl, m);
-
- PKTCNTR_INC(&cl->cl_stats.xmit_cnt, m->m_pkthdr.len);
-
- hfsc_update_vf(cl, m->m_pkthdr.len, cur_time);
+ hfsc_update_vf(cl, len, cur_time);
if (realtime)
- cl->cl_cumul += m->m_pkthdr.len;
+ cl->cl_cumul += len;
if (hfsc_class_qlength(cl) > 0) {
/*
* Realtime queue needs to look into the future and make
* calculations based on that. This is the reason it can't
* be used with an external queue manager.
*/
if (cl->cl_rsc != NULL) {
+ struct mbuf *m0;
+
/* update ed */
- m0 = hfsc_class_deq_begin(cl, &free_ml);
- ifq_mfreeml(ifq, &free_ml);
+ KASSERT(cl->cl_qops == pfq_hfsc_ops);
+ m0 = MBUF_LIST_FIRST(&cl->cl_q.q);
next_len = m0->m_pkthdr.len;
if (realtime)
hfsc_update_ed(hif, cl, next_len);
else
