[dpdk-dev,1/2] net: fixup RARP generation

Message ID 1516245283-23990-1-git-send-email-yliu@fridaylinux.org (mailing list archive)
State Accepted, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Yuanhan Liu Jan. 18, 2018, 3:14 a.m. UTC
  Due to a mistake operation from me, older version (v10) was merged to
master branch. It's the v11 should be applied. However, the master branch
is not rebase-able. Thus, this patch is made, from the diff between v10
and v11.

Code is from Xiao Wang.

Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
---
 lib/librte_net/rte_arp.c      | 26 +++++++++++++++++---------
 lib/librte_net/rte_arp.h      | 11 ++++++-----
 lib/librte_vhost/virtio_net.c | 12 +++---------
 3 files changed, 26 insertions(+), 23 deletions(-)
  

Comments

Thomas Monjalon Jan. 18, 2018, 8:38 a.m. UTC | #1
18/01/2018 04:14, Yuanhan Liu:
> Due to a mistake operation from me, older version (v10) was merged to
> master branch. It's the v11 should be applied. However, the master branch
> is not rebase-able. Thus, this patch is made, from the diff between v10
> and v11.

Understood it is a mistake.
However, you can briefly describes what does this change.
Is there a changelog in v11 patch?
> 
> Code is from Xiao Wang.

You may add his Signed-off.

> Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
  
Yuanhan Liu Jan. 18, 2018, 8:51 a.m. UTC | #2
On Thu, Jan 18, 2018 at 09:38:39AM +0100, Thomas Monjalon wrote:
> 18/01/2018 04:14, Yuanhan Liu:
> > Due to a mistake operation from me, older version (v10) was merged to
> > master branch. It's the v11 should be applied. However, the master branch
> > is not rebase-able. Thus, this patch is made, from the diff between v10
> > and v11.
> 
> Understood it is a mistake.
> However, you can briefly describes what does this change.
> Is there a changelog in v11 patch?

Yes, ther is:

v11:
- Add check for parameter and tailroom in rte_net_make_rarp_packet.
- Allocate mbuf in rte_net_make_rarp_packet.

> > 
> > Code is from Xiao Wang.
> 
> You may add his Signed-off.

I have no objection. Xiao, okay to you? I will also set the author
to you.

	--yliu

> > Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
  
Xiao Wang Jan. 18, 2018, 8:53 a.m. UTC | #3
> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Thursday, January 18, 2018 4:51 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Wang, Xiao W <xiao.w.wang@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Olivier Matz <olivier.matz@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH 1/2] net: fixup RARP generation
> 
> On Thu, Jan 18, 2018 at 09:38:39AM +0100, Thomas Monjalon wrote:
> > 18/01/2018 04:14, Yuanhan Liu:
> > > Due to a mistake operation from me, older version (v10) was merged to
> > > master branch. It's the v11 should be applied. However, the master branch
> > > is not rebase-able. Thus, this patch is made, from the diff between v10
> > > and v11.
> >
> > Understood it is a mistake.
> > However, you can briefly describes what does this change.
> > Is there a changelog in v11 patch?
> 
> Yes, ther is:
> 
> v11:
> - Add check for parameter and tailroom in rte_net_make_rarp_packet.
> - Allocate mbuf in rte_net_make_rarp_packet.
> 
> > >
> > > Code is from Xiao Wang.
> >
> > You may add his Signed-off.
> 
> I have no objection. Xiao, okay to you? I will also set the author
> to you.
> 
> 	--yliu
> 
> > > Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>

OK for me.

BRs,
Xiao
  
Ferruh Yigit Jan. 19, 2018, 4:04 p.m. UTC | #4
On 1/18/2018 3:14 AM, Yuanhan Liu wrote:
> Due to a mistake operation from me, older version (v10) was merged to
> master branch. It's the v11 should be applied. However, the master branch
> is not rebase-able. Thus, this patch is made, from the diff between v10
> and v11.
> 
> Code is from Xiao Wang.

    Fixes: 45ae05df824c ("net: add a helper for making RARP packet")
    Fixes: c3ffdba0e88a ("vhost: use API to make RARP packet")

Please correct me if there are wrong.

> 
> Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
> ---
>  lib/librte_net/rte_arp.c      | 26 +++++++++++++++++---------
>  lib/librte_net/rte_arp.h      | 11 ++++++-----
>  lib/librte_vhost/virtio_net.c | 12 +++---------
>  3 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/librte_net/rte_arp.c b/lib/librte_net/rte_arp.c
> index d7223b0..b953bcd 100644
> --- a/lib/librte_net/rte_arp.c
> +++ b/lib/librte_net/rte_arp.c
> @@ -7,17 +7,28 @@
>  #include <rte_arp.h>
>  
>  #define RARP_PKT_SIZE	64
> -int
> -rte_net_make_rarp_packet(struct rte_mbuf *mbuf, const struct ether_addr *mac)
> +struct rte_mbuf *
> +rte_net_make_rarp_packet(struct rte_mempool *mpool,
> +		const struct ether_addr *mac)
>  {
>  	struct ether_hdr *eth_hdr;
>  	struct arp_hdr *rarp;
> +	struct rte_mbuf *mbuf;
>  
> -	if (mbuf->buf_len < RARP_PKT_SIZE)
> -		return -1;
> +	if (mpool == NULL)
> +		return NULL;
> +
> +	mbuf = rte_pktmbuf_alloc(mpool);
> +	if (mbuf == NULL)
> +		return NULL;
> +
> +	eth_hdr = (struct ether_hdr *)rte_pktmbuf_append(mbuf, RARP_PKT_SIZE);
> +	if (eth_hdr == NULL) {
> +		rte_pktmbuf_free(mbuf);
> +		return NULL;
> +	}
>  
>  	/* Ethernet header. */
> -	eth_hdr = rte_pktmbuf_mtod(mbuf, struct ether_hdr *);
>  	memset(eth_hdr->d_addr.addr_bytes, 0xff, ETHER_ADDR_LEN);
>  	ether_addr_copy(mac, &eth_hdr->s_addr);
>  	eth_hdr->ether_type = htons(ETHER_TYPE_RARP);
> @@ -35,8 +46,5 @@ rte_net_make_rarp_packet(struct rte_mbuf *mbuf, const struct ether_addr *mac)
>  	memset(&rarp->arp_data.arp_sip, 0x00, 4);
>  	memset(&rarp->arp_data.arp_tip, 0x00, 4);
>  
> -	mbuf->data_len = RARP_PKT_SIZE;
> -	mbuf->pkt_len = RARP_PKT_SIZE;
> -
> -	return 0;
> +	return mbuf;
>  }
> diff --git a/lib/librte_net/rte_arp.h b/lib/librte_net/rte_arp.h
> index dad7423..457a39b 100644
> --- a/lib/librte_net/rte_arp.h
> +++ b/lib/librte_net/rte_arp.h
> @@ -82,16 +82,17 @@ struct arp_hdr {
>   *
>   * Make a RARP packet based on MAC addr.
>   *
> - * @param mbuf
> - *   Pointer to the rte_mbuf structure
> + * @param mpool
> + *   Pointer to the rte_mempool
>   * @param mac
>   *   Pointer to the MAC addr
>   *
>   * @return
> - *   - 0 on success, negative on error
> + *   - RARP packet pointer on success, or NULL on error
>   */
> -int
> -rte_net_make_rarp_packet(struct rte_mbuf *mbuf, const struct ether_addr *mac);
> +struct rte_mbuf *
> +rte_net_make_rarp_packet(struct rte_mempool *mpool,
> +		const struct ether_addr *mac);
>  
>  #ifdef __cplusplus
>  }
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index ca89288..a1d8026 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -1162,19 +1162,13 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
>  			rte_atomic16_cmpset((volatile uint16_t *)
>  				&dev->broadcast_rarp.cnt, 1, 0))) {
>  
> -		rarp_mbuf = rte_pktmbuf_alloc(mbuf_pool);
> +		rarp_mbuf = rte_net_make_rarp_packet(mbuf_pool, &dev->mac);
>  		if (rarp_mbuf == NULL) {
>  			RTE_LOG(ERR, VHOST_DATA,
> -				"Failed to allocate memory for mbuf.\n");
> +				"Failed to make RARP packet.\n");
>  			return 0;
>  		}
> -
> -		if (rte_net_make_rarp_packet(rarp_mbuf, &dev->mac) < 0) {
> -			rte_pktmbuf_free(rarp_mbuf);
> -			rarp_mbuf = NULL;
> -		} else {
> -			count -= 1;
> -		}
> +		count -= 1;
>  	}
>  
>  	free_entries = *((volatile uint16_t *)&vq->avail->idx) -
>
  

Patch

diff --git a/lib/librte_net/rte_arp.c b/lib/librte_net/rte_arp.c
index d7223b0..b953bcd 100644
--- a/lib/librte_net/rte_arp.c
+++ b/lib/librte_net/rte_arp.c
@@ -7,17 +7,28 @@ 
 #include <rte_arp.h>
 
 #define RARP_PKT_SIZE	64
-int
-rte_net_make_rarp_packet(struct rte_mbuf *mbuf, const struct ether_addr *mac)
+struct rte_mbuf *
+rte_net_make_rarp_packet(struct rte_mempool *mpool,
+		const struct ether_addr *mac)
 {
 	struct ether_hdr *eth_hdr;
 	struct arp_hdr *rarp;
+	struct rte_mbuf *mbuf;
 
-	if (mbuf->buf_len < RARP_PKT_SIZE)
-		return -1;
+	if (mpool == NULL)
+		return NULL;
+
+	mbuf = rte_pktmbuf_alloc(mpool);
+	if (mbuf == NULL)
+		return NULL;
+
+	eth_hdr = (struct ether_hdr *)rte_pktmbuf_append(mbuf, RARP_PKT_SIZE);
+	if (eth_hdr == NULL) {
+		rte_pktmbuf_free(mbuf);
+		return NULL;
+	}
 
 	/* Ethernet header. */
-	eth_hdr = rte_pktmbuf_mtod(mbuf, struct ether_hdr *);
 	memset(eth_hdr->d_addr.addr_bytes, 0xff, ETHER_ADDR_LEN);
 	ether_addr_copy(mac, &eth_hdr->s_addr);
 	eth_hdr->ether_type = htons(ETHER_TYPE_RARP);
@@ -35,8 +46,5 @@  rte_net_make_rarp_packet(struct rte_mbuf *mbuf, const struct ether_addr *mac)
 	memset(&rarp->arp_data.arp_sip, 0x00, 4);
 	memset(&rarp->arp_data.arp_tip, 0x00, 4);
 
-	mbuf->data_len = RARP_PKT_SIZE;
-	mbuf->pkt_len = RARP_PKT_SIZE;
-
-	return 0;
+	return mbuf;
 }
diff --git a/lib/librte_net/rte_arp.h b/lib/librte_net/rte_arp.h
index dad7423..457a39b 100644
--- a/lib/librte_net/rte_arp.h
+++ b/lib/librte_net/rte_arp.h
@@ -82,16 +82,17 @@  struct arp_hdr {
  *
  * Make a RARP packet based on MAC addr.
  *
- * @param mbuf
- *   Pointer to the rte_mbuf structure
+ * @param mpool
+ *   Pointer to the rte_mempool
  * @param mac
  *   Pointer to the MAC addr
  *
  * @return
- *   - 0 on success, negative on error
+ *   - RARP packet pointer on success, or NULL on error
  */
-int
-rte_net_make_rarp_packet(struct rte_mbuf *mbuf, const struct ether_addr *mac);
+struct rte_mbuf *
+rte_net_make_rarp_packet(struct rte_mempool *mpool,
+		const struct ether_addr *mac);
 
 #ifdef __cplusplus
 }
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index ca89288..a1d8026 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -1162,19 +1162,13 @@  rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
 			rte_atomic16_cmpset((volatile uint16_t *)
 				&dev->broadcast_rarp.cnt, 1, 0))) {
 
-		rarp_mbuf = rte_pktmbuf_alloc(mbuf_pool);
+		rarp_mbuf = rte_net_make_rarp_packet(mbuf_pool, &dev->mac);
 		if (rarp_mbuf == NULL) {
 			RTE_LOG(ERR, VHOST_DATA,
-				"Failed to allocate memory for mbuf.\n");
+				"Failed to make RARP packet.\n");
 			return 0;
 		}
-
-		if (rte_net_make_rarp_packet(rarp_mbuf, &dev->mac) < 0) {
-			rte_pktmbuf_free(rarp_mbuf);
-			rarp_mbuf = NULL;
-		} else {
-			count -= 1;
-		}
+		count -= 1;
 	}
 
 	free_entries = *((volatile uint16_t *)&vq->avail->idx) -