On Mon, Oct 21, 2013 at 12:04:14AM +0200, Martin Pelikan wrote:
> Hopefully the third time does the charm.
>
> The previous union approach to altq/newq bits was wrong, because
> switching back and forth was racy. This new diff then concatenates
> these structures like [ifqueue, hfsc_if, altq-bits], has some better
> names, doesn't need renaming stuff in the old code (it can remain 2
> years old), removes the now useless ifq_hfsc pointer.
Can't we not just nuke altq? It is going to die anyway so why try to keep
it alive?
> As always, it's being heavily tested.
> --
> Martin Pelikan
>
>
> ? net/hfsc.c.instrumented
> Index: altq/if_altq.h
> ===================================================================
> RCS file: /cvs/src/sys/altq/if_altq.h,v
> retrieving revision 1.16
> diff -u -p -r1.16 if_altq.h
> --- altq/if_altq.h 12 Oct 2013 12:13:10 -0000 1.16
> +++ altq/if_altq.h 20 Oct 2013 21:25:41 -0000
> @@ -36,6 +36,13 @@ struct altq_pktattr; struct oldtb_regula
> /*
> * Structure defining a queue for a network interface.
> */
> +
> +/* XXX hack, because we need the structure definition */
> +#define ALTQ_IS_ENABLED 1
> +#include <net/hfsc.h>
> +#undef ALTQ_IS_ENABLED
> +/* XXX hack */
> +
> struct ifaltq {
> /* fields compatible with struct ifqueue */
> struct {
> @@ -45,8 +52,8 @@ struct ifaltq {
> int ifq_len;
> int ifq_maxlen;
> int ifq_drops;
> - struct hfsc_if *ifq_hfsc;
> struct timeout *ifq_congestion;
> + struct hfsc_if ifq_hfsc;
>
> /* alternate queueing related fields */
> int altq_type; /* discipline type */
> Index: net/hfsc.c
> ===================================================================
> RCS file: /cvs/src/sys/net/hfsc.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 hfsc.c
> --- net/hfsc.c 12 Oct 2013 11:39:17 -0000 1.1
> +++ net/hfsc.c 20 Oct 2013 21:25:42 -0000
> @@ -63,7 +63,7 @@
> /*
> * function prototypes
> */
> -struct hfsc_class *hfsc_class_create(struct hfsc_if *,
> +struct hfsc_class *hfsc_class_create(struct ifqueue *,
> struct hfsc_sc *, struct hfsc_sc *,
> struct hfsc_sc *, struct hfsc_class *, int,
> int, int);
> @@ -128,18 +128,49 @@ hfsc_microuptime(void)
> HFSC_CLK_SHIFT);
> }
>
> +/*
> + * The new table will be exactly one page larger, so in the most
> + * common case of 8B pointers and 4KB pages it's 512 more classes.
> + * Returns the amount of classes, so all new pages are 100% utilized.
> + */
> +static inline u_int
> +hfsc_more_slots(u_int current)
> +{
> + u_int was_pages = current * sizeof(void *) / PAGE_SIZE;
> + u_int n = ((was_pages + 1) * PAGE_SIZE) / sizeof(void *);
> +
> + return (n);
> +}
> +
> +static void
> +hfsc_grow_class_tbl(struct hfsc_if *hif)
> +{
> + struct hfsc_class **newtbl, **old = hif->hif_class_tbl;
> + const u_int slots = hfsc_more_slots(hif->hif_allocated);
> +
> + newtbl = malloc(slots * sizeof(void *), M_DEVBUF, M_WAITOK | M_ZERO);
> + memcpy(newtbl, old, hif->hif_allocated * sizeof(void *));
> +
> + hif->hif_allocated = slots;
> + hif->hif_class_tbl = newtbl;
> +
> + free(old, M_DEVBUF);
> +}
> +
> int
> hfsc_attach(struct ifnet *ifp)
> {
> - struct hfsc_if *hif;
> + const u_int slots = hfsc_more_slots(0);
> + const size_t sz = slots * sizeof(void *);
> + struct hfsc_if *hif = &ifp->if_snd.ifq_hfsc;
>
> - if (ifp->if_snd.ifq_hfsc != NULL)
> + if (hif->hif_class_tbl != NULL)
> return (0);
>
> - hif = malloc(sizeof(struct hfsc_if), M_DEVBUF, M_WAITOK|M_ZERO);
> + hif->hif_class_tbl = malloc(sz, M_DEVBUF, M_WAITOK | M_ZERO);
> + hif->hif_allocated = slots;
> + hif->hif_classes = 0;
> hif->hif_eligible = hfsc_ellist_alloc();
> - hif->hif_ifq = (struct ifqueue *)&ifp->if_snd; /* XXX cast temp */
> - ifp->if_snd.ifq_hfsc = hif;
>
> return (0);
> }
> @@ -147,8 +178,11 @@ hfsc_attach(struct ifnet *ifp)
> int
> hfsc_detach(struct ifnet *ifp)
> {
> - free(ifp->if_snd.ifq_hfsc, M_DEVBUF);
> - ifp->if_snd.ifq_hfsc = NULL;
> + struct hfsc_if *hif = &ifp->if_snd.ifq_hfsc;
> +
> + hif->hif_allocated = hif->hif_classes = 0;
> + free(hif->hif_class_tbl, M_DEVBUF);
> + hif->hif_class_tbl = NULL;
>
> return (0);
> }
> @@ -156,11 +190,13 @@ hfsc_detach(struct ifnet *ifp)
> int
> hfsc_addqueue(struct pf_queuespec *q)
> {
> - struct hfsc_if *hif;
> + /* XXX remove the cast when ifaltq is gone. */
> + struct ifqueue *ifq = (struct ifqueue *)&q->kif->pfik_ifp->if_snd;
> + struct hfsc_if *hif = &ifq->ifq_hfsc;
> struct hfsc_class *cl, *parent;
> struct hfsc_sc rtsc, lssc, ulsc;
>
> - if ((hif = q->kif->pfik_ifp->if_snd.ifq_hfsc) == NULL)
> + if (hif->hif_allocated == 0)
> return (EINVAL);
>
> if (q->parent_qid == HFSC_NULLCLASS_HANDLE &&
> @@ -185,7 +221,7 @@ hfsc_addqueue(struct pf_queuespec *q)
> ulsc.d = q->upperlimit.d;
> ulsc.m2 = q->upperlimit.m2.absolute;
>
> - cl = hfsc_class_create(hif, &rtsc, &lssc, &ulsc,
> + cl = hfsc_class_create(ifq, &rtsc, &lssc, &ulsc,
> parent, q->qlimit, q->flags, q->qid);
> if (cl == NULL)
> return (ENOMEM);
> @@ -196,10 +232,10 @@ hfsc_addqueue(struct pf_queuespec *q)
> int
> hfsc_delqueue(struct pf_queuespec *q)
> {
> - struct hfsc_if *hif;
> + struct hfsc_if *hif = &q->kif->pfik_ifp->if_snd.ifq_hfsc;
> struct hfsc_class *cl;
>
> - if ((hif = q->kif->pfik_ifp->if_snd.ifq_hfsc) == NULL)
> + if (hif->hif_allocated == 0)
> return (EINVAL);
>
> if ((cl = hfsc_clh2cph(hif, q->qid)) == NULL)
> @@ -211,12 +247,12 @@ hfsc_delqueue(struct pf_queuespec *q)
> int
> hfsc_qstats(struct pf_queuespec *q, void *ubuf, int *nbytes)
> {
> - struct hfsc_if *hif;
> + struct hfsc_if *hif = &q->kif->pfik_ifp->if_snd.ifq_hfsc;
> struct hfsc_class *cl;
> struct hfsc_class_stats stats;
> int error = 0;
>
> - if ((hif = q->kif->pfik_ifp->if_snd.ifq_hfsc) == NULL)
> + if (hif->hif_allocated == 0)
> return (EBADF);
>
> if ((cl = hfsc_clh2cph(hif, q->qid)) == NULL)
> @@ -236,25 +272,26 @@ hfsc_qstats(struct pf_queuespec *q, void
> void
> hfsc_purge(struct ifqueue *ifq)
> {
> - struct hfsc_if *hif = ifq->ifq_hfsc;
> + struct hfsc_if *hif = &ifq->ifq_hfsc;
> struct hfsc_class *cl;
>
> for (cl = hif->hif_rootclass; cl != NULL; cl = hfsc_nextclass(cl))
> if (cl->cl_q->qlen > 0)
> hfsc_purgeq(cl);
> - hif->hif_ifq->ifq_len = 0;
> + ifq->ifq_len = 0;
> }
>
> struct hfsc_class *
> -hfsc_class_create(struct hfsc_if *hif, struct hfsc_sc *rsc,
> +hfsc_class_create(struct ifqueue *ifq, struct hfsc_sc *rsc,
> struct hfsc_sc *fsc, struct hfsc_sc *usc, struct hfsc_class *parent,
> int qlimit, int flags, int qid)
> {
> + struct hfsc_if *hif = &ifq->ifq_hfsc;
> struct hfsc_class *cl, *p;
> int i, s;
>
> - if (hif->hif_classes >= HFSC_MAX_CLASSES)
> - return (NULL);
> + if (hif->hif_classes >= hif->hif_allocated)
> + hfsc_grow_class_tbl(hif);
>
> cl = malloc(sizeof(struct hfsc_class), M_DEVBUF, M_WAITOK|M_ZERO);
> cl->cl_q = malloc(sizeof(struct hfsc_classq), M_DEVBUF,
> @@ -285,7 +322,7 @@ hfsc_class_create(struct hfsc_if *hif, s
> if (m2 < 8)
> red_pkttime = 1000 * 1000 * 1000; /* 1 sec */
> else
> - red_pkttime = (int64_t)hif->hif_ifq->altq_ifp->if_mtu
> + red_pkttime = (int64_t)ifq->altq_ifp->if_mtu
> * 1000 * 1000 * 1000 / (m2 / 8);
> if (flags & HFSC_RED) {
> cl->cl_red = red_alloc(0, 0,
> @@ -319,7 +356,7 @@ hfsc_class_create(struct hfsc_if *hif, s
>
> cl->cl_id = hif->hif_classid++;
> cl->cl_handle = qid;
> - cl->cl_hif = hif;
> + cl->cl_ifq = ifq;
> cl->cl_parent = parent;
>
> s = splnet();
> @@ -330,16 +367,16 @@ hfsc_class_create(struct hfsc_if *hif, s
> * the lower bits of qid is free, use this slot. otherwise,
> * use the first free slot.
> */
> - i = qid % HFSC_MAX_CLASSES;
> + i = qid % hif->hif_allocated;
> if (hif->hif_class_tbl[i] == NULL)
> hif->hif_class_tbl[i] = cl;
> else {
> - for (i = 0; i < HFSC_MAX_CLASSES; i++)
> + for (i = 0; i < hif->hif_allocated; i++)
> if (hif->hif_class_tbl[i] == NULL) {
> hif->hif_class_tbl[i] = cl;
> break;
> }
> - if (i == HFSC_MAX_CLASSES) {
> + if (i == hif->hif_allocated) {
> splx(s);
> goto err_ret;
> }
> @@ -388,6 +425,7 @@ err_ret:
> int
> hfsc_class_destroy(struct hfsc_class *cl)
> {
> + struct hfsc_if *hif = &cl->cl_ifq->ifq_hfsc;
> int i, s;
>
> if (cl == NULL)
> @@ -414,13 +452,13 @@ hfsc_class_destroy(struct hfsc_class *cl
> } while ((p = p->cl_siblings) != NULL);
> }
>
> - for (i = 0; i < HFSC_MAX_CLASSES; i++)
> - if (cl->cl_hif->hif_class_tbl[i] == cl) {
> - cl->cl_hif->hif_class_tbl[i] = NULL;
> + for (i = 0; i < hif->hif_allocated; i++)
> + if (hif->hif_class_tbl[i] == cl) {
> + hif->hif_class_tbl[i] = NULL;
> break;
> }
>
> - cl->cl_hif->hif_classes--;
> + hif->hif_classes--;
> splx(s);
>
> hfsc_actlist_destroy(cl->cl_actc);
> @@ -432,10 +470,10 @@ hfsc_class_destroy(struct hfsc_class *cl
> }
> #endif
>
> - if (cl == cl->cl_hif->hif_rootclass)
> - cl->cl_hif->hif_rootclass = NULL;
> - if (cl == cl->cl_hif->hif_defaultclass)
> - cl->cl_hif->hif_defaultclass = NULL;
> + if (cl == cl->cl_ifq->ifq_hfsc.hif_rootclass)
> + cl->cl_ifq->ifq_hfsc.hif_rootclass = NULL;
> + if (cl == cl->cl_ifq->ifq_hfsc.hif_defaultclass)
> + cl->cl_ifq->ifq_hfsc.hif_defaultclass = NULL;
>
> if (cl->cl_usc != NULL)
> free(cl->cl_usc, M_DEVBUF);
> @@ -476,7 +514,7 @@ hfsc_nextclass(struct hfsc_class *cl)
> int
> hfsc_enqueue(struct ifqueue *ifq, struct mbuf *m)
> {
> - struct hfsc_if *hif = ifq->ifq_hfsc;
> + struct hfsc_if *hif = &ifq->ifq_hfsc;
> struct hfsc_class *cl;
>
> if ((cl = hfsc_clh2cph(hif, m->m_pkthdr.pf.qid)) == NULL ||
> @@ -496,7 +534,7 @@ hfsc_enqueue(struct ifqueue *ifq, struct
> return (ENOBUFS);
> }
> IFQ_INC_LEN(ifq);
> - cl->cl_hif->hif_packets++;
> + cl->cl_ifq->ifq_hfsc.hif_packets++;
> m->m_pkthdr.pf.prio = IFQ_MAXPRIO;
>
> /* successfully queued. */
> @@ -509,7 +547,7 @@ hfsc_enqueue(struct ifqueue *ifq, struct
> struct mbuf *
> hfsc_dequeue(struct ifqueue *ifq, int remove)
> {
> - struct hfsc_if *hif = ifq->ifq_hfsc;
> + struct hfsc_if *hif = &ifq->ifq_hfsc;
> struct hfsc_class *cl, *tcl;
> struct mbuf *m;
> int next_len, realtime = 0;
> @@ -577,7 +615,7 @@ hfsc_dequeue(struct ifqueue *ifq, int re
> if (m == NULL)
> panic("hfsc_dequeue");
>
> - cl->cl_hif->hif_packets--;
> + cl->cl_ifq->ifq_hfsc.hif_packets--;
> IFQ_DEC_LEN(ifq);
> PKTCNTR_INC(&cl->cl_stats.xmit_cnt, m->m_pkthdr.len);
>
> @@ -661,8 +699,8 @@ hfsc_purgeq(struct hfsc_class *cl)
> while ((m = hfsc_getq(cl)) != NULL) {
> PKTCNTR_INC(&cl->cl_stats.drop_cnt, m->m_pkthdr.len);
> m_freem(m);
> - cl->cl_hif->hif_packets--;
> - IFQ_DEC_LEN(cl->cl_hif->hif_ifq);
> + cl->cl_ifq->ifq_hfsc.hif_packets--;
> + IFQ_DEC_LEN(cl->cl_ifq);
> }
>
> hfsc_update_vf(cl, 0, 0); /* remove cl from the actlist */
> @@ -955,7 +993,7 @@ hfsc_ellist_destroy(hfsc_ellist_t *head)
> void
> hfsc_ellist_insert(struct hfsc_class *cl)
> {
> - struct hfsc_if *hif = cl->cl_hif;
> + struct hfsc_if *hif = &cl->cl_ifq->ifq_hfsc;
> struct hfsc_class *p;
>
> /* check the last entry first */
> @@ -976,7 +1014,7 @@ hfsc_ellist_insert(struct hfsc_class *cl
> void
> hfsc_ellist_remove(struct hfsc_class *cl)
> {
> - struct hfsc_if *hif = cl->cl_hif;
> + struct hfsc_if *hif = &cl->cl_ifq->ifq_hfsc;
>
> TAILQ_REMOVE(hif->hif_eligible, cl, cl_ellist);
> }
> @@ -984,7 +1022,7 @@ hfsc_ellist_remove(struct hfsc_class *cl
> void
> hfsc_ellist_update(struct hfsc_class *cl)
> {
> - struct hfsc_if *hif = cl->cl_hif;
> + struct hfsc_if *hif = &cl->cl_ifq->ifq_hfsc;
> struct hfsc_class *p, *last;
>
> /*
> @@ -1454,10 +1492,10 @@ hfsc_clh2cph(struct hfsc_if *hif, u_int3
> * first, try the slot corresponding to the lower bits of the handle.
> * if it does not match, do the linear table search.
> */
> - i = chandle % HFSC_MAX_CLASSES;
> + i = chandle % hif->hif_allocated;
> if ((cl = hif->hif_class_tbl[i]) != NULL && cl->cl_handle == chandle)
> return (cl);
> - for (i = 0; i < HFSC_MAX_CLASSES; i++)
> + for (i = 0; i < hif->hif_allocated; i++)
> if ((cl = hif->hif_class_tbl[i]) != NULL &&
> cl->cl_handle == chandle)
> return (cl);
> Index: net/hfsc.h
> ===================================================================
> RCS file: /cvs/src/sys/net/hfsc.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 hfsc.h
> --- net/hfsc.h 12 Oct 2013 11:39:17 -0000 1.1
> +++ net/hfsc.h 20 Oct 2013 21:25:42 -0000
> @@ -55,7 +55,6 @@ struct hfsc_sc {
>
> /* special class handles */
> #define HFSC_NULLCLASS_HANDLE 0
> -#define HFSC_MAX_CLASSES 64
>
> /* service curve types */
> #define HFSC_REALTIMESC 1
> @@ -168,7 +167,7 @@ typedef TAILQ_ENTRY(hfsc_class) hfsc_act
> struct hfsc_class {
> u_int cl_id; /* class id (just for debug) */
> u_int32_t cl_handle; /* class handle */
> - struct hfsc_if *cl_hif; /* back pointer to struct hfsc_if */
> + struct ifqueue *cl_ifq; /* back pointer to our ifqueue */
> int cl_flags; /* misc flags */
>
> struct hfsc_class *cl_parent; /* parent class */
> @@ -230,13 +229,12 @@ struct hfsc_class {
> * hfsc interface state
> */
> struct hfsc_if {
> - struct hfsc_if *hif_next; /* interface state list */
> - struct ifqueue *hif_ifq; /* backpointer to ifq */
> + struct hfsc_class **hif_class_tbl;
> struct hfsc_class *hif_rootclass; /* root class */
> struct hfsc_class *hif_defaultclass; /* default class */
> - struct hfsc_class *hif_class_tbl[HFSC_MAX_CLASSES];
> struct hfsc_class *hif_pollcache; /* cache for poll operation */
>
> + u_int hif_allocated; /* how many slots above */
> u_int hif_classes; /* # of classes in the tree */
> u_int hif_packets; /* # of packets in the tree */
> u_int hif_classid; /* class id sequence number */
> @@ -248,7 +246,7 @@ struct hfsc_if {
> #define HFSC_FREQ (1000000 << HFSC_CLK_SHIFT)
> #define HFSC_CLK_PER_TICK (HFSC_FREQ / hz)
>
> -#define HFSC_ENABLED(ifq) ((ifq)->ifq_hfsc != NULL)
> +#define HFSC_ENABLED(ifq) ((ifq)->ifq_hfsc.hif_class_tbl != NULL)
> #define HFSC_DEFAULT_QLIMIT 50
>
> /* XXX hack */
> Index: net/if.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if.h,v
> retrieving revision 1.149
> diff -u -p -r1.149 if.h
> --- net/if.h 19 Oct 2013 14:05:14 -0000 1.149
> +++ net/if.h 20 Oct 2013 21:25:42 -0000
> @@ -184,8 +184,8 @@ struct ifqueue {
> int ifq_len;
> int ifq_maxlen;
> int ifq_drops;
> - struct hfsc_if *ifq_hfsc;
> struct timeout *ifq_congestion;
> + struct hfsc_if ifq_hfsc;
> };
>
> /*
>
--
:wq Claudio
