[dpdk-dev] [PATCH 1/3] kni: minor opto

[dpdk-dev] [PATCH 1/3] kni: minor opto

[Jay Rolette]

Don't need the 'safe' version of list_for_each_entry() if you aren't deleting from the list as you iterate over it

Signed-off-by: Jay Rolette <rolette@infiniteio.com>
---
 lib/librte_eal/linuxapp/kni/kni_misc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 1935d32..312f196 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -213,13 +213,12 @@ static int
 kni_thread_single(void *unused)
 {
 	int j;
-	struct kni_dev *dev, *n;
+	struct kni_dev *dev;
 
 	while (!kthread_should_stop()) {
 		down_read(&kni_list_lock);
 		for (j = 0; j < KNI_RX_LOOP_NUM; j++) {
-			list_for_each_entry_safe(dev, n,
-					&kni_list_head, list) {
+			list_for_each_entry(dev, &kni_list_head, list) {
 #ifdef RTE_KNI_VHOST
 				kni_chk_vhost_rx(dev);
 #else
-- 
2.3.2 (Apple Git-55)

[dpdk-dev] [PATCH 2/3] kni: minor opto

[Jay Rolette]

No reason to check out many entries are in kni->rx_q prior to
actually pulling them from the fifo. You can't dequeue more than
are there anyway. Max entries to dequeue is either the max batch
size or however much space is available on kni->free_q (lesser of the two)

Signed-off-by: Jay Rolette <rolette@infiniteio.com>
---
 lib/librte_eal/linuxapp/kni/kni_net.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c b/lib/librte_eal/linuxapp/kni/kni_net.c
index dd95db5..13ccbb8 100644
--- a/lib/librte_eal/linuxapp/kni/kni_net.c
+++ b/lib/librte_eal/linuxapp/kni/kni_net.c
@@ -131,7 +131,7 @@ kni_net_rx_normal(struct kni_dev *kni)
 {
 	unsigned ret;
 	uint32_t len;
-	unsigned i, num, num_rq, num_fq;
+	unsigned i, num, num_fq;
 	struct rte_kni_mbuf *kva;
 	struct rte_kni_mbuf *va[MBUF_BURST_SZ];
 	void * data_kva;
@@ -139,24 +139,19 @@ kni_net_rx_normal(struct kni_dev *kni)
 	struct sk_buff *skb;
 	struct net_device *dev = kni->net_dev;
 
-	/* Get the number of entries in rx_q */
-	num_rq = kni_fifo_count(kni->rx_q);
-
 	/* Get the number of free entries in free_q */
-	num_fq = kni_fifo_free_count(kni->free_q);
-
-	/* Calculate the number of entries to dequeue in rx_q */
-	num = min(num_rq, num_fq);
-	num = min(num, (unsigned)MBUF_BURST_SZ);
-
-	/* Return if no entry in rx_q and no free entry in free_q */
-	if (num == 0)
+	if ((num_fq = kni_fifo_free_count(kni->free_q)) == 0) {
+		/* No room on the free_q, bail out */
 		return;
+	}
+
+	/* Calculate the number of entries to dequeue from rx_q */
+	num = min(num_fq, (unsigned)MBUF_BURST_SZ);
 
 	/* Burst dequeue from rx_q */
 	ret = kni_fifo_get(kni->rx_q, (void **)va, num);
 	if (ret == 0)
-		return; /* Failing should not happen */
+		return;
 
 	/* Transfer received packets to netif */
 	for (i = 0; i < num; i++) {
-- 
2.3.2 (Apple Git-55)

Re: [dpdk-dev] [PATCH 2/3] kni: minor opto

[Bruce Richardson]

On Wed, Jun 03, 2015 at 02:18:56PM -0500, Jay Rolette wrote:
> No reason to check out many entries are in kni->rx_q prior to
> actually pulling them from the fifo. You can't dequeue more than
> are there anyway. Max entries to dequeue is either the max batch
> size or however much space is available on kni->free_q (lesser of the two)
> 
> Signed-off-by: Jay Rolette <rolette@infiniteio.com>

Again title needs to be made clearer. Otherwise ok.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH 2/3] kni: minor opto

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jay Rolette
> Sent: Thursday, June 4, 2015 3:19 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/3] kni: minor opto
The title is not so descriptive for the changes.
The change itself seems OK for me. Thanks!

- Helin

> 
> No reason to check out many entries are in kni->rx_q prior to actually pulling
> them from the fifo. You can't dequeue more than are there anyway. Max entries
> to dequeue is either the max batch size or however much space is available on
> kni->free_q (lesser of the two)
> 
> Signed-off-by: Jay Rolette <rolette@infiniteio.com>
> ---
>  lib/librte_eal/linuxapp/kni/kni_net.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c
> b/lib/librte_eal/linuxapp/kni/kni_net.c
> index dd95db5..13ccbb8 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_net.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_net.c
> @@ -131,7 +131,7 @@ kni_net_rx_normal(struct kni_dev *kni)  {
>  	unsigned ret;
>  	uint32_t len;
> -	unsigned i, num, num_rq, num_fq;
> +	unsigned i, num, num_fq;
>  	struct rte_kni_mbuf *kva;
>  	struct rte_kni_mbuf *va[MBUF_BURST_SZ];
>  	void * data_kva;
> @@ -139,24 +139,19 @@ kni_net_rx_normal(struct kni_dev *kni)
>  	struct sk_buff *skb;
>  	struct net_device *dev = kni->net_dev;
> 
> -	/* Get the number of entries in rx_q */
> -	num_rq = kni_fifo_count(kni->rx_q);
> -
>  	/* Get the number of free entries in free_q */
> -	num_fq = kni_fifo_free_count(kni->free_q);
> -
> -	/* Calculate the number of entries to dequeue in rx_q */
> -	num = min(num_rq, num_fq);
> -	num = min(num, (unsigned)MBUF_BURST_SZ);
> -
> -	/* Return if no entry in rx_q and no free entry in free_q */
> -	if (num == 0)
> +	if ((num_fq = kni_fifo_free_count(kni->free_q)) == 0) {
> +		/* No room on the free_q, bail out */
>  		return;
> +	}
> +
> +	/* Calculate the number of entries to dequeue from rx_q */
> +	num = min(num_fq, (unsigned)MBUF_BURST_SZ);
> 
>  	/* Burst dequeue from rx_q */
>  	ret = kni_fifo_get(kni->rx_q, (void **)va, num);
>  	if (ret == 0)
> -		return; /* Failing should not happen */
> +		return;
> 
>  	/* Transfer received packets to netif */
>  	for (i = 0; i < num; i++) {
> --
> 2.3.2 (Apple Git-55)

[dpdk-dev] [PATCH 3/3] kni: rx loop was using the wrong counter

[Jay Rolette]

Loop processing packets dequeued from rx_q was using the number of
packets requested, not how many it actually received.

Variable rename to make code a little more clear

Signed-off-by: Jay Rolette <rolette@infiniteio.com>
---
 lib/librte_eal/linuxapp/kni/kni_net.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c b/lib/librte_eal/linuxapp/kni/kni_net.c
index 13ccbb8..d37a6b9 100644
--- a/lib/librte_eal/linuxapp/kni/kni_net.c
+++ b/lib/librte_eal/linuxapp/kni/kni_net.c
@@ -131,7 +131,7 @@ kni_net_rx_normal(struct kni_dev *kni)
 {
 	unsigned ret;
 	uint32_t len;
-	unsigned i, num, num_fq;
+	unsigned i, num_rx, num_fq;
 	struct rte_kni_mbuf *kva;
 	struct rte_kni_mbuf *va[MBUF_BURST_SZ];
 	void * data_kva;
@@ -146,15 +146,15 @@ kni_net_rx_normal(struct kni_dev *kni)
 	}
 
 	/* Calculate the number of entries to dequeue from rx_q */
-	num = min(num_fq, (unsigned)MBUF_BURST_SZ);
+	num_rx = min(num_fq, (unsigned)MBUF_BURST_SZ);
 
 	/* Burst dequeue from rx_q */
-	ret = kni_fifo_get(kni->rx_q, (void **)va, num);
-	if (ret == 0)
+	num_rx = kni_fifo_get(kni->rx_q, (void **)va, num_rx);
+	if (num_rx == 0)
 		return;
 
 	/* Transfer received packets to netif */
-	for (i = 0; i < num; i++) {
+	for (i = 0; i < num_rx; i++) {
 		kva = (void *)va[i] - kni->mbuf_va + kni->mbuf_kva;
 		len = kva->data_len;
 		data_kva = kva->buf_addr + kva->data_off - kni->mbuf_va
@@ -184,8 +184,8 @@ kni_net_rx_normal(struct kni_dev *kni)
 	}
 
 	/* Burst enqueue mbufs into free_q */
-	ret = kni_fifo_put(kni->free_q, (void **)va, num);
-	if (ret != num)
+	ret = kni_fifo_put(kni->free_q, (void **)va, num_rx);
+	if (ret != num_rx)
 		/* Failing should not happen */
 		KNI_ERR("Fail to enqueue entries into free_q\n");
 }
-- 
2.3.2 (Apple Git-55)

Re: [dpdk-dev] [PATCH 3/3] kni: rx loop was using the wrong counter

[Bruce Richardson]

On Wed, Jun 03, 2015 at 02:18:57PM -0500, Jay Rolette wrote:
> Loop processing packets dequeued from rx_q was using the number of
> packets requested, not how many it actually received.
> 
> Variable rename to make code a little more clear
> 
> Signed-off-by: Jay Rolette <rolette@infiniteio.com>

s/more clear/clearer/

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

[Bruce Richardson]

On Wed, Jun 03, 2015 at 02:18:55PM -0500, Jay Rolette wrote:
> Don't need the 'safe' version of list_for_each_entry() if you aren't deleting from the list as you iterate over it
> 
> Signed-off-by: Jay Rolette <rolette@infiniteio.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

[Bruce Richardson]

On Thu, Jun 04, 2015 at 02:39:17PM +0100, Bruce Richardson wrote:
> On Wed, Jun 03, 2015 at 02:18:55PM -0500, Jay Rolette wrote:
> > Don't need the 'safe' version of list_for_each_entry() if you aren't deleting from the list as you iterate over it
> > 
> > Signed-off-by: Jay Rolette <rolette@infiniteio.com>
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
Forgot to mention though, that the commit title needs to be a little more
descriptive.

Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

[Thomas Monjalon]

2015-06-04 14:40, Bruce Richardson:
> On Thu, Jun 04, 2015 at 02:39:17PM +0100, Bruce Richardson wrote:
> > On Wed, Jun 03, 2015 at 02:18:55PM -0500, Jay Rolette wrote:
> > > Don't need the 'safe' version of list_for_each_entry() if you aren't deleting from the list as you iterate over it
> > > 
> > > Signed-off-by: Jay Rolette <rolette@infiniteio.com>
> > 
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> Forgot to mention though, that the commit title needs to be a little more
> descriptive.

So you should not ack this version ;)

Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

[Bruce Richardson]

On Thu, Jun 04, 2015 at 05:02:06PM +0200, Thomas Monjalon wrote:
> 2015-06-04 14:40, Bruce Richardson:
> > On Thu, Jun 04, 2015 at 02:39:17PM +0100, Bruce Richardson wrote:
> > > On Wed, Jun 03, 2015 at 02:18:55PM -0500, Jay Rolette wrote:
> > > > Don't need the 'safe' version of list_for_each_entry() if you aren't deleting from the list as you iterate over it
> > > > 
> > > > Signed-off-by: Jay Rolette <rolette@infiniteio.com>
> > > 
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > 
> > Forgot to mention though, that the commit title needs to be a little more
> > descriptive.
> 
> So you should not ack this version ;)
> 
Code and text description looked fine, so I thought an ack otherwise appropriate.
However, I will refrain from doing so in future :-)

/Bruce

Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

[Zhang, Helin]

Would it be better to modify the similar thing in kni_ioctl_create()?

- Helin

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jay Rolette
> Sent: Thursday, June 4, 2015 3:19 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/3] kni: minor opto
> 
> Don't need the 'safe' version of list_for_each_entry() if you aren't deleting from
> the list as you iterate over it
> 
> Signed-off-by: Jay Rolette <rolette@infiniteio.com>
> ---
>  lib/librte_eal/linuxapp/kni/kni_misc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
> b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 1935d32..312f196 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -213,13 +213,12 @@ static int
>  kni_thread_single(void *unused)
>  {
>  	int j;
> -	struct kni_dev *dev, *n;
> +	struct kni_dev *dev;
> 
>  	while (!kthread_should_stop()) {
>  		down_read(&kni_list_lock);
>  		for (j = 0; j < KNI_RX_LOOP_NUM; j++) {
> -			list_for_each_entry_safe(dev, n,
> -					&kni_list_head, list) {
> +			list_for_each_entry(dev, &kni_list_head, list) {
>  #ifdef RTE_KNI_VHOST
>  				kni_chk_vhost_rx(dev);
>  #else
> --
> 2.3.2 (Apple Git-55)

Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

[Jay Rolette]

On Sun, Jun 14, 2015 at 9:07 PM, Zhang, Helin <helin.zhang@intel.com> wrote:

> Would it be better to modify the similar thing in kni_ioctl_create()?
>

That one doesn't need to use the "safe" version of list_for_each_entry()
either, but it isn't in the packet processing path so the minor performance
improvement doesn't really matter.


>
> - Helin
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jay Rolette
> > Sent: Thursday, June 4, 2015 3:19 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 1/3] kni: minor opto
> >
> > Don't need the 'safe' version of list_for_each_entry() if you aren't
> deleting from
> > the list as you iterate over it
> >
> > Signed-off-by: Jay Rolette <rolette@infiniteio.com>
> > ---
> >  lib/librte_eal/linuxapp/kni/kni_misc.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > index 1935d32..312f196 100644
> > --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > @@ -213,13 +213,12 @@ static int
> >  kni_thread_single(void *unused)
> >  {
> >       int j;
> > -     struct kni_dev *dev, *n;
> > +     struct kni_dev *dev;
> >
> >       while (!kthread_should_stop()) {
> >               down_read(&kni_list_lock);
> >               for (j = 0; j < KNI_RX_LOOP_NUM; j++) {
> > -                     list_for_each_entry_safe(dev, n,
> > -                                     &kni_list_head, list) {
> > +                     list_for_each_entry(dev, &kni_list_head, list) {
> >  #ifdef RTE_KNI_VHOST
> >                               kni_chk_vhost_rx(dev);
> >  #else
> > --
> > 2.3.2 (Apple Git-55)
>
>

Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

[Zhang, Helin]

Hi Jay

From: Jay Rolette [mailto:rolette@infiniteio.com]
Sent: Monday, June 15, 2015 8:43 PM
To: Zhang, Helin
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

On Sun, Jun 14, 2015 at 9:07 PM, Zhang, Helin <helin.zhang@intel.com<mailto:helin.zhang@intel.com>> wrote:
Would it be better to modify the similar thing in kni_ioctl_create()?

That one doesn't need to use the "safe" version of list_for_each_entry() either, but it isn't in the packet processing path so the minor performance improvement doesn't really matter.
Yes, your patches are OK for me. I have acked it.


-          Helin


- Helin

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org<mailto:dev-bounces@dpdk.org>] On Behalf Of Jay Rolette
> Sent: Thursday, June 4, 2015 3:19 AM
> To: dev@dpdk.org<mailto:dev@dpdk.org>
> Subject: [dpdk-dev] [PATCH 1/3] kni: minor opto
>
> Don't need the 'safe' version of list_for_each_entry() if you aren't deleting from
> the list as you iterate over it
>
> Signed-off-by: Jay Rolette <rolette@infiniteio.com<mailto:rolette@infiniteio.com>>
> ---
>  lib/librte_eal/linuxapp/kni/kni_misc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
> b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 1935d32..312f196 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -213,13 +213,12 @@ static int
>  kni_thread_single(void *unused)
>  {
>       int j;
> -     struct kni_dev *dev, *n;
> +     struct kni_dev *dev;
>
>       while (!kthread_should_stop()) {
>               down_read(&kni_list_lock);
>               for (j = 0; j < KNI_RX_LOOP_NUM; j++) {
> -                     list_for_each_entry_safe(dev, n,
> -                                     &kni_list_head, list) {
> +                     list_for_each_entry(dev, &kni_list_head, list) {
>  #ifdef RTE_KNI_VHOST
>                               kni_chk_vhost_rx(dev);
>  #else
> --
> 2.3.2 (Apple Git-55)

Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

[Thomas Monjalon]

2015-06-16 01:15, Zhang, Helin:
> Yes, your patches are OK for me. I have acked it.

Series applied, thanks

[dpdk-dev] [PATCH 1/3] kni: minor opto

[Jay Rolette]

Don't need the 'safe' version of list_for_each_entry() if you aren't deleting from the list as you iterate over it

Signed-off-by: Jay Rolette <rolette@infiniteio.com>
---
 lib/librte_eal/linuxapp/kni/kni_misc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 1935d32..312f196 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -213,13 +213,12 @@ static int
 kni_thread_single(void *unused)
 {
 	int j;
-	struct kni_dev *dev, *n;
+	struct kni_dev *dev;
 
 	while (!kthread_should_stop()) {
 		down_read(&kni_list_lock);
 		for (j = 0; j < KNI_RX_LOOP_NUM; j++) {
-			list_for_each_entry_safe(dev, n,
-					&kni_list_head, list) {
+			list_for_each_entry(dev, &kni_list_head, list) {
 #ifdef RTE_KNI_VHOST
 				kni_chk_vhost_rx(dev);
 #else
-- 
2.3.2 (Apple Git-55)

Re: [dpdk-dev] [PATCH 1/3] kni: minor opto

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jay Rolette
> Sent: Thursday, June 4, 2015 3:08 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/3] kni: minor opto
> 
> Don't need the 'safe' version of list_for_each_entry() if you aren't deleting from
> the list as you iterate over it
> 
> Signed-off-by: Jay Rolette <rolette@infiniteio.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>

> ---
>  lib/librte_eal/linuxapp/kni/kni_misc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
> b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 1935d32..312f196 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -213,13 +213,12 @@ static int
>  kni_thread_single(void *unused)
>  {
>  	int j;
> -	struct kni_dev *dev, *n;
> +	struct kni_dev *dev;
> 
>  	while (!kthread_should_stop()) {
>  		down_read(&kni_list_lock);
>  		for (j = 0; j < KNI_RX_LOOP_NUM; j++) {
> -			list_for_each_entry_safe(dev, n,
> -					&kni_list_head, list) {
> +			list_for_each_entry(dev, &kni_list_head, list) {
>  #ifdef RTE_KNI_VHOST
>  				kni_chk_vhost_rx(dev);
>  #else
> --
> 2.3.2 (Apple Git-55)