priq: convert to mbuf lists

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

priq: convert to mbuf lists

Mike Belopuhov-5
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 = &priq_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->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->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->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->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

Reply | Threaded
Open this post in threaded view
|

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->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->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@

Reply | Threaded
Open this post in threaded view
|

Re: priq: convert to mbuf lists

David Gwynne-5
In reply to this post by Mike Belopuhov-5

> On 2 Mar 2017, at 06:43, Mike Belopuhov <[hidden email]> 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.

>
> ---
> 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 = &priq_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->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->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->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->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
>

Reply | Threaded
Open this post in threaded view
|

Re: priq: convert to mbuf lists

Mike Belopuhov-5
On 2 March 2017 at 00:56, David Gwynne <[hidden email]> wrote:

>
>> On 2 Mar 2017, at 06:43, Mike Belopuhov <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> wrote:
> >
> >> On 2 Mar 2017, at 06:43, Mike Belopuhov <[hidden email]> 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@

Reply | Threaded
Open this post in threaded view
|

Re: priq: convert to mbuf lists

Mike Belopuhov-5
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 <[hidden email]> wrote:
> > >
> > >> On 2 Mar 2017, at 06:43, Mike Belopuhov <[hidden email]> 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 = &priq_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->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->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->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->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

Reply | Threaded
Open this post in threaded view
|

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@

Reply | Threaded
Open this post in threaded view
|

Re: priq: convert to mbuf lists

David Gwynne-5
In reply to this post by Mike Belopuhov-5

> On 2 Mar 2017, at 21:19, Mike Belopuhov <[hidden email]> 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 <[hidden email]> wrote:
>>>>
>>>>> On 2 Mar 2017, at 06:43, Mike Belopuhov <[hidden email]> 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 = &priq_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->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->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->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->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
>

Reply | Threaded
Open this post in threaded view
|

Re: priq: convert to mbuf lists

Mike Belopuhov-5
On 7 March 2017 at 02:32, David Gwynne <[hidden email]> wrote:

>
>> On 2 Mar 2017, at 21:19, Mike Belopuhov <[hidden email]> 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 <[hidden email]> wrote:
>>>>>
>>>>>> On 2 Mar 2017, at 06:43, Mike Belopuhov <[hidden email]> 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).