Re: priq: convert to mbuf lists

[Mike Belopuhov]

On 7 March 2017 at 02:32, David Gwynne  wrote:
>
>> On 2 Mar 2017, at 21:19, Mike Belopuhov  wrote:
>>
>> On Thu, Mar 02, 2017 at 10:11 +0100, Martin Pieuchot wrote:
>>> On 02/03/17(Thu) 01:16, Mike Belopuhov wrote:
 On 2 March 2017 at 00:56, David Gwynne  wrote:
>
>> On 2 Mar 2017, at 06:43, Mike Belopuhov  wrote:
>>
>> This convers hand rolled lists into exactly the same mbuf_lists.
>> I need this because of the next diff that uses the ml_len packet
>> counter that mbuf_lists have.  Otherwise there's no functional
>> change.
>
> i didnt use mbuf lists here because they have an extra counter
> that isnt, or wasnt, needed.
>
> im not sure you need to know how long a list is in your later
> diff, you just need to know if it is not empty. you can do that
> by checking if the head is NULL.
>

 true, i was thinking about clarifying this, but i like the
 "declarativeness" of the length check.  i don't think that an
 extra counter is a big deal.  also mbuf lists make this code look
 simpler which is a good thing, imo.
>>>
>>> I agree.  I find the code much easier to understand with mikeb@'s diff.
>>>
>>> IMHO we should avoid hand-rolled lists.
>>>
>>> ok mpi@
>>>
>>
>> Here's an updated diff with suggestions from bluhm@ and it's
>> even slimmer now but I don't mind either way.  If David isn't
>> comfortable with this change, we don't have to do it.
>
> im really not keen, but i guess i will submit to populism if
> everyone else feels strongly about this.
>

I've pondered it a bit more and while I didn't think I care too
much, while rebasing I've realised that I'd be doing a disservice
for everyone coming later to this code.  What tipped the scales
was how you used priq_deq_commit in the last diff for the lack
of better API and I thought hell, there is a better API: mbuf_lists.
It's just much more fitting.  You've even invented it yourself!

And besides I'm of opinion that priq_deq_begin and priq_deq_commit
existence is purely accidental and they should be merged and drivers
using them are doing it wrong from the QoS point of view (albeit not
so much from the priq PoV).

Re: priq: convert to mbuf lists

[David Gwynne]

> On 2 Mar 2017, at 21:19, Mike Belopuhov  wrote:
> 
> On Thu, Mar 02, 2017 at 10:11 +0100, Martin Pieuchot wrote:
>> On 02/03/17(Thu) 01:16, Mike Belopuhov wrote:
>>> On 2 March 2017 at 00:56, David Gwynne  wrote:
 
> On 2 Mar 2017, at 06:43, Mike Belopuhov  wrote:
> 
> This convers hand rolled lists into exactly the same mbuf_lists.
> I need this because of the next diff that uses the ml_len packet
> counter that mbuf_lists have.  Otherwise there's no functional
> change.
 
 i didnt use mbuf lists here because they have an extra counter
 that isnt, or wasnt, needed.
 
 im not sure you need to know how long a list is in your later
 diff, you just need to know if it is not empty. you can do that
 by checking if the head is NULL.
 
>>> 
>>> true, i was thinking about clarifying this, but i like the
>>> "declarativeness" of the length check.  i don't think that an
>>> extra counter is a big deal.  also mbuf lists make this code look
>>> simpler which is a good thing, imo.
>> 
>> I agree.  I find the code much easier to understand with mikeb@'s diff.
>> 
>> IMHO we should avoid hand-rolled lists.
>> 
>> ok mpi@
>> 
> 
> Here's an updated diff with suggestions from bluhm@ and it's
> even slimmer now but I don't mind either way.  If David isn't
> comfortable with this change, we don't have to do it.

im really not keen, but i guess i will submit to populism if everyone else 
feels strongly about this.

> 
> ---
> sys/net/ifq.c | 50 ++
> 1 file changed, 18 insertions(+), 32 deletions(-)
> 
> diff --git sys/net/ifq.c sys/net/ifq.c
> index 40731637a2c..a601e9b9bd1 100644
> --- sys/net/ifq.c
> +++ sys/net/ifq.c
> @@ -51,17 +51,12 @@ const struct ifq_ops * const ifq_priq_ops = _ops;
> 
> /*
>  * priq internal structures
>  */
> 
> -struct priq_list {
> - struct mbuf *head;
> - struct mbuf *tail;
> -};
> -
> struct priq {
> - struct priq_list pq_lists[IFQ_NQUEUES];
> + struct mbuf_list pq_lists[IFQ_NQUEUES];
> };
> 
> /*
>  * ifqueue serialiser
>  */
> @@ -392,11 +387,17 @@ priq_idx(unsigned int nqueues, const struct mbuf *m)
> }
> 
> void *
> priq_alloc(unsigned int idx, void *null)
> {
> - return (malloc(sizeof(struct priq), M_DEVBUF, M_WAITOK | M_ZERO));
> + struct priq *pq;
> + int i;
> +
> + pq = malloc(sizeof(struct priq), M_DEVBUF, M_WAITOK);
> + for (i = 0; i < IFQ_NQUEUES; i++)
> + ml_init(>pq_lists[i]);
> + return (pq);
> }
> 
> void
> priq_free(unsigned int idx, void *pq)
> {
> @@ -405,40 +406,35 @@ priq_free(unsigned int idx, void *pq)
> 
> int
> priq_enq(struct ifqueue *ifq, struct mbuf *m)
> {
>   struct priq *pq;
> - struct priq_list *pl;
> + struct mbuf_list *pl;
> 
>   if (ifq_len(ifq) >= ifq->ifq_maxlen)
>   return (ENOBUFS);
> 
>   pq = ifq->ifq_q;
>   KASSERT(m->m_pkthdr.pf.prio <= IFQ_MAXPRIO);
>   pl = >pq_lists[m->m_pkthdr.pf.prio];
> 
> - m->m_nextpkt = NULL;
> - if (pl->tail == NULL)
> - pl->head = m;
> - else
> - pl->tail->m_nextpkt = m;
> - pl->tail = m;
> + ml_enqueue(pl, m);
> 
>   return (0);
> }
> 
> struct mbuf *
> priq_deq_begin(struct ifqueue *ifq, void **cookiep)
> {
>   struct priq *pq = ifq->ifq_q;
> - struct priq_list *pl;
> + struct mbuf_list *pl;
>   unsigned int prio = nitems(pq->pq_lists);
>   struct mbuf *m;
> 
>   do {
>   pl = >pq_lists[--prio];
> - m = pl->head;
> + m = MBUF_LIST_FIRST(pl);
>   if (m != NULL) {
>   *cookiep = pl;
>   return (m);
>   }
>   } while (prio > 0);
> @@ -447,35 +443,25 @@ priq_deq_begin(struct ifqueue *ifq, void **cookiep)
> }
> 
> void
> priq_deq_commit(struct ifqueue *ifq, struct mbuf *m, void *cookie)
> {
> - struct priq_list *pl = cookie;
> -
> - KASSERT(pl->head == m);
> + struct mbuf_list *pl = cookie;
> 
> - pl->head = m->m_nextpkt;
> - m->m_nextpkt = NULL;
> + KASSERT(MBUF_LIST_FIRST(pl) == m);
> 
> - if (pl->head == NULL)
> - pl->tail = NULL;
> + ml_dequeue(pl);
> }
> 
> void
> priq_purge(struct ifqueue *ifq, struct mbuf_list *ml)
> {
>   struct priq *pq = ifq->ifq_q;
> - struct priq_list *pl;
> + struct mbuf_list *pl;
>   unsigned int prio = nitems(pq->pq_lists);
> - struct mbuf *m, *n;
> + struct mbuf *m;
> 
>   do {
>   pl = >pq_lists[--prio];
> -
> - for (m = pl->head; m != NULL; m = n) {
> - n = m->m_nextpkt;
> - ml_enqueue(ml, m);
> - }
> -
> - pl->head = pl->tail = NULL;
> + ml_enlist(ml, pl);
>   } while (prio > 0);
> }
> -- 
> 2.12.0
> 

Re: priq: convert to mbuf lists

[Alexander Bluhm]

On Thu, Mar 02, 2017 at 12:19:44PM +0100, Mike Belopuhov wrote:
> Here's an updated diff with suggestions from bluhm@ and it's
> even slimmer now but I don't mind either way.  If David isn't
> comfortable with this change, we don't have to do it.

OK bluhm@

Re: priq: convert to mbuf lists

[Mike Belopuhov]

On Thu, Mar 02, 2017 at 10:11 +0100, Martin Pieuchot wrote:
> On 02/03/17(Thu) 01:16, Mike Belopuhov wrote:
> > On 2 March 2017 at 00:56, David Gwynne  wrote:
> > >
> > >> On 2 Mar 2017, at 06:43, Mike Belopuhov  wrote:
> > >>
> > >> This convers hand rolled lists into exactly the same mbuf_lists.
> > >> I need this because of the next diff that uses the ml_len packet
> > >> counter that mbuf_lists have.  Otherwise there's no functional
> > >> change.
> > >
> > > i didnt use mbuf lists here because they have an extra counter
> > > that isnt, or wasnt, needed.
> > >
> > > im not sure you need to know how long a list is in your later
> > > diff, you just need to know if it is not empty. you can do that
> > > by checking if the head is NULL.
> > >
> > 
> > true, i was thinking about clarifying this, but i like the
> > "declarativeness" of the length check.  i don't think that an
> > extra counter is a big deal.  also mbuf lists make this code look
> > simpler which is a good thing, imo.
> 
> I agree.  I find the code much easier to understand with mikeb@'s diff.
> 
> IMHO we should avoid hand-rolled lists.
> 
> ok mpi@
> 

Here's an updated diff with suggestions from bluhm@ and it's
even slimmer now but I don't mind either way.  If David isn't
comfortable with this change, we don't have to do it.

---
 sys/net/ifq.c | 50 ++
 1 file changed, 18 insertions(+), 32 deletions(-)

diff --git sys/net/ifq.c sys/net/ifq.c
index 40731637a2c..a601e9b9bd1 100644
--- sys/net/ifq.c
+++ sys/net/ifq.c
@@ -51,17 +51,12 @@ const struct ifq_ops * const ifq_priq_ops = _ops;
 
 /*
  * priq internal structures
  */
 
-struct priq_list {
-   struct mbuf *head;
-   struct mbuf *tail;
-};
-
 struct priq {
-   struct priq_list pq_lists[IFQ_NQUEUES];
+   struct mbuf_list pq_lists[IFQ_NQUEUES];
 };
 
 /*
  * ifqueue serialiser
  */
@@ -392,11 +387,17 @@ priq_idx(unsigned int nqueues, const struct mbuf *m)
 }
 
 void *
 priq_alloc(unsigned int idx, void *null)
 {
-   return (malloc(sizeof(struct priq), M_DEVBUF, M_WAITOK | M_ZERO));
+   struct priq *pq;
+   int i;
+
+   pq = malloc(sizeof(struct priq), M_DEVBUF, M_WAITOK);
+   for (i = 0; i < IFQ_NQUEUES; i++)
+   ml_init(>pq_lists[i]);
+   return (pq);
 }
 
 void
 priq_free(unsigned int idx, void *pq)
 {
@@ -405,40 +406,35 @@ priq_free(unsigned int idx, void *pq)
 
 int
 priq_enq(struct ifqueue *ifq, struct mbuf *m)
 {
struct priq *pq;
-   struct priq_list *pl;
+   struct mbuf_list *pl;
 
if (ifq_len(ifq) >= ifq->ifq_maxlen)
return (ENOBUFS);
 
pq = ifq->ifq_q;
KASSERT(m->m_pkthdr.pf.prio <= IFQ_MAXPRIO);
pl = >pq_lists[m->m_pkthdr.pf.prio];
 
-   m->m_nextpkt = NULL;
-   if (pl->tail == NULL)
-   pl->head = m;
-   else
-   pl->tail->m_nextpkt = m;
-   pl->tail = m;
+   ml_enqueue(pl, m);
 
return (0);
 }
 
 struct mbuf *
 priq_deq_begin(struct ifqueue *ifq, void **cookiep)
 {
struct priq *pq = ifq->ifq_q;
-   struct priq_list *pl;
+   struct mbuf_list *pl;
unsigned int prio = nitems(pq->pq_lists);
struct mbuf *m;
 
do {
pl = >pq_lists[--prio];
-   m = pl->head;
+   m = MBUF_LIST_FIRST(pl);
if (m != NULL) {
*cookiep = pl;
return (m);
}
} while (prio > 0);
@@ -447,35 +443,25 @@ priq_deq_begin(struct ifqueue *ifq, void **cookiep)
 }
 
 void
 priq_deq_commit(struct ifqueue *ifq, struct mbuf *m, void *cookie)
 {
-   struct priq_list *pl = cookie;
-
-   KASSERT(pl->head == m);
+   struct mbuf_list *pl = cookie;
 
-   pl->head = m->m_nextpkt;
-   m->m_nextpkt = NULL;
+   KASSERT(MBUF_LIST_FIRST(pl) == m);
 
-   if (pl->head == NULL)
-   pl->tail = NULL;
+   ml_dequeue(pl);
 }
 
 void
 priq_purge(struct ifqueue *ifq, struct mbuf_list *ml)
 {
struct priq *pq = ifq->ifq_q;
-   struct priq_list *pl;
+   struct mbuf_list *pl;
unsigned int prio = nitems(pq->pq_lists);
-   struct mbuf *m, *n;
+   struct mbuf *m;
 
do {
pl = >pq_lists[--prio];
-
-   for (m = pl->head; m != NULL; m = n) {
-   n = m->m_nextpkt;
-   ml_enqueue(ml, m);
-   }
-
-   pl->head = pl->tail = NULL;
+   ml_enlist(ml, pl);
} while (prio > 0);
 }
-- 
2.12.0

Re: priq: convert to mbuf lists

[Martin Pieuchot]

On 02/03/17(Thu) 01:16, Mike Belopuhov wrote:
> On 2 March 2017 at 00:56, David Gwynne  wrote:
> >
> >> On 2 Mar 2017, at 06:43, Mike Belopuhov  wrote:
> >>
> >> This convers hand rolled lists into exactly the same mbuf_lists.
> >> I need this because of the next diff that uses the ml_len packet
> >> counter that mbuf_lists have.  Otherwise there's no functional
> >> change.
> >
> > i didnt use mbuf lists here because they have an extra counter that isnt, 
> > or wasnt, needed.
> >
> > im not sure you need to know how long a list is in your later diff, you 
> > just need to know if it is not empty. you can do that by checking if the 
> > head is NULL.
> >
> 
> true, i was thinking about clarifying this, but i like the "declarativeness"
> of the length check.  i don't think that an extra counter is a big deal.
> also mbuf lists make this code look simpler which is a good thing, imo.

I agree.  I find the code much easier to understand with mikeb@'s diff.

IMHO we should avoid hand-rolled lists.

ok mpi@

Re: priq: convert to mbuf lists

[Mike Belopuhov]

On 2 March 2017 at 00:56, David Gwynne  wrote:
>
>> On 2 Mar 2017, at 06:43, Mike Belopuhov  wrote:
>>
>> This convers hand rolled lists into exactly the same mbuf_lists.
>> I need this because of the next diff that uses the ml_len packet
>> counter that mbuf_lists have.  Otherwise there's no functional
>> change.
>
> i didnt use mbuf lists here because they have an extra counter that isnt, or 
> wasnt, needed.
>
> im not sure you need to know how long a list is in your later diff, you just 
> need to know if it is not empty. you can do that by checking if the head is 
> NULL.
>

true, i was thinking about clarifying this, but i like the "declarativeness"
of the length check.  i don't think that an extra counter is a big deal.
also mbuf lists make this code look simpler which is a good thing, imo.

Re: priq: convert to mbuf lists

[Alexander Bluhm]

On Wed, Mar 01, 2017 at 09:43:16PM +0100, Mike Belopuhov wrote:
> This convers hand rolled lists into exactly the same mbuf_lists.
> I need this because of the next diff that uses the ml_len packet
> counter that mbuf_lists have.  Otherwise there's no functional
> change.

>  void *
>  priq_alloc(unsigned int idx, void *null)
>  {
> - return (malloc(sizeof(struct priq), M_DEVBUF, M_WAITOK | M_ZERO));
> + struct priq *pq;
> + int i;
> +
> + pq = malloc(sizeof(struct priq), M_DEVBUF, M_WAITOK | M_ZERO);

You don't need M_ZERO as you call ml_init() afterwards.

> + for (i = 0; i < IFQ_NQUEUES; i++)
> + ml_init(>pq_lists[i]);
> + return (pq);
>  }

>  void
>  priq_purge(struct ifqueue *ifq, struct mbuf_list *ml)
>  {
>   struct priq *pq = ifq->ifq_q;
> - struct priq_list *pl;
> + struct mbuf_list *pl;
>   unsigned int prio = nitems(pq->pq_lists);
> - struct mbuf *m, *n;
> + struct mbuf *m;
>  
>   do {
>   pl = >pq_lists[--prio];
>  
> - for (m = pl->head; m != NULL; m = n) {
> - n = m->m_nextpkt;
> + while ((m = ml_dequeue(pl)) != NULL)
>   ml_enqueue(ml, m);

This is equivalent to ml_enlist(ml, pl) which avoids the loop.

> - }
> -
> - pl->head = pl->tail = NULL;
>   } while (prio > 0);
>  }

with that OK bluhm@

priq: convert to mbuf lists

[Mike Belopuhov]

This convers hand rolled lists into exactly the same mbuf_lists.
I need this because of the next diff that uses the ml_len packet
counter that mbuf_lists have.  Otherwise there's no functional
change.

---
 sys/net/ifq.c | 48 ++--
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git sys/net/ifq.c sys/net/ifq.c
index 40731637a2c..896b373c454 100644
--- sys/net/ifq.c
+++ sys/net/ifq.c
@@ -51,17 +51,12 @@ const struct ifq_ops * const ifq_priq_ops = _ops;
 
 /*
  * priq internal structures
  */
 
-struct priq_list {
-   struct mbuf *head;
-   struct mbuf *tail;
-};
-
 struct priq {
-   struct priq_list pq_lists[IFQ_NQUEUES];
+   struct mbuf_list pq_lists[IFQ_NQUEUES];
 };
 
 /*
  * ifqueue serialiser
  */
@@ -392,11 +387,17 @@ priq_idx(unsigned int nqueues, const struct mbuf *m)
 }
 
 void *
 priq_alloc(unsigned int idx, void *null)
 {
-   return (malloc(sizeof(struct priq), M_DEVBUF, M_WAITOK | M_ZERO));
+   struct priq *pq;
+   int i;
+
+   pq = malloc(sizeof(struct priq), M_DEVBUF, M_WAITOK | M_ZERO);
+   for (i = 0; i < IFQ_NQUEUES; i++)
+   ml_init(>pq_lists[i]);
+   return (pq);
 }
 
 void
 priq_free(unsigned int idx, void *pq)
 {
@@ -405,40 +406,35 @@ priq_free(unsigned int idx, void *pq)
 
 int
 priq_enq(struct ifqueue *ifq, struct mbuf *m)
 {
struct priq *pq;
-   struct priq_list *pl;
+   struct mbuf_list *pl;
 
if (ifq_len(ifq) >= ifq->ifq_maxlen)
return (ENOBUFS);
 
pq = ifq->ifq_q;
KASSERT(m->m_pkthdr.pf.prio <= IFQ_MAXPRIO);
pl = >pq_lists[m->m_pkthdr.pf.prio];
 
-   m->m_nextpkt = NULL;
-   if (pl->tail == NULL)
-   pl->head = m;
-   else
-   pl->tail->m_nextpkt = m;
-   pl->tail = m;
+   ml_enqueue(pl, m);
 
return (0);
 }
 
 struct mbuf *
 priq_deq_begin(struct ifqueue *ifq, void **cookiep)
 {
struct priq *pq = ifq->ifq_q;
-   struct priq_list *pl;
+   struct mbuf_list *pl;
unsigned int prio = nitems(pq->pq_lists);
struct mbuf *m;
 
do {
pl = >pq_lists[--prio];
-   m = pl->head;
+   m = MBUF_LIST_FIRST(pl);
if (m != NULL) {
*cookiep = pl;
return (m);
}
} while (prio > 0);
@@ -447,35 +443,27 @@ priq_deq_begin(struct ifqueue *ifq, void **cookiep)
 }
 
 void
 priq_deq_commit(struct ifqueue *ifq, struct mbuf *m, void *cookie)
 {
-   struct priq_list *pl = cookie;
+   struct mbuf_list *pl = cookie;
 
-   KASSERT(pl->head == m);
+   KASSERT(MBUF_LIST_FIRST(pl) == m);
 
-   pl->head = m->m_nextpkt;
-   m->m_nextpkt = NULL;
-
-   if (pl->head == NULL)
-   pl->tail = NULL;
+   ml_dequeue(pl);
 }
 
 void
 priq_purge(struct ifqueue *ifq, struct mbuf_list *ml)
 {
struct priq *pq = ifq->ifq_q;
-   struct priq_list *pl;
+   struct mbuf_list *pl;
unsigned int prio = nitems(pq->pq_lists);
-   struct mbuf *m, *n;
+   struct mbuf *m;
 
do {
pl = >pq_lists[--prio];
 
-   for (m = pl->head; m != NULL; m = n) {
-   n = m->m_nextpkt;
+   while ((m = ml_dequeue(pl)) != NULL)
ml_enqueue(ml, m);
-   }
-
-   pl->head = pl->tail = NULL;
} while (prio > 0);
 }
-- 
2.12.0