Message ID | 1508159716-3656-3-git-send-email-antonio.fischetti@intel.com |
---|---|
State | Superseded |
Headers | show |
Series | netdev-dpdk: Fix mempool management and other cleanup. | expand |
>From: Fischetti, Antonio >Sent: Monday, October 16, 2017 2:15 PM >To: dev@openvswitch.org >Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Darrell Ball ><dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>; Kevin Traynor ><ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com>; Fischetti, Antonio ><antonio.fischetti@intel.com> >Subject: [PATCH v6 2/5] netdev-dpdk: skip init for existing mempools. > >Skip initialization of mempool packet areas if this was already >done in a previous call to dpdk_mp_create. Hi Antonio, As stated in my previous review, I believe that this could probably be folded into patch 1 of the series (it was patch 3 of v5). However, I don't object strongly to this patch, so I'll leave it to your discretion. Other than that, LGTM. Thanks, Mark > >CC: Mark B Kavanagh <mark.b.kavanagh@intel.com> >CC: Darrell Ball <dlu998@gmail.com> >CC: Ciara Loftus <ciara.loftus@intel.com> >CC: Kevin Traynor <ktraynor@redhat.com> >CC: Aaron Conole <aconole@redhat.com> >Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> >--- > lib/netdev-dpdk.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >index 7f2d7ed..07c438a 100644 >--- a/lib/netdev-dpdk.c >+++ b/lib/netdev-dpdk.c >@@ -550,6 +550,11 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool >*mp_exists) > if (dmp->mp) { > VLOG_DBG("Allocated \"%s\" mempool with %u mbufs", mp_name, > dmp->mp_size); >+ /* rte_pktmbuf_pool_create has done some initialization of the >+ * rte_mbuf part of each dp_packet. Some OvS specific fields >+ * of the packet still need to be initialized by >+ * ovs_rte_pktmbuf_init. */ >+ rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL); > } else if (rte_errno == EEXIST) { > /* A mempool with the same name already exists. We just > * retrieve its pointer to be returned to the caller. */ >@@ -566,11 +571,6 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool >*mp_exists) > } > free(mp_name); > if (dmp->mp) { >- /* rte_pktmbuf_pool_create has done some initialization of the >- * rte_mbuf part of each dp_packet, while ovs_rte_pktmbuf_init >- * initializes some OVS specific fields of dp_packet. >- */ >- rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL); > return dmp; > } > } while (!(*mp_exists) && >-- >2.4.11
> -----Original Message----- > From: Kavanagh, Mark B > Sent: Tuesday, October 17, 2017 2:34 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > Cc: Darrell Ball <dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>; > Kevin Traynor <ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com> > Subject: RE: [PATCH v6 2/5] netdev-dpdk: skip init for existing mempools. > > >From: Fischetti, Antonio > >Sent: Monday, October 16, 2017 2:15 PM > >To: dev@openvswitch.org > >Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Darrell Ball > ><dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>; Kevin Traynor > ><ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com>; Fischetti, Antonio > ><antonio.fischetti@intel.com> > >Subject: [PATCH v6 2/5] netdev-dpdk: skip init for existing mempools. > > > >Skip initialization of mempool packet areas if this was already > >done in a previous call to dpdk_mp_create. > > Hi Antonio, > > As stated in my previous review, I believe that this could probably be folded > into patch 1 of the series (it was patch 3 of v5). > However, I don't object strongly to this patch, so I'll leave it to your > discretion. [Antonio] I'm keeping this change in a separate patch because it is not related to the fixes for the mempool management. It's a small improvement, not so much to save CPU cycles, it's actually a clean up of the code. > > Other than that, LGTM. > > Thanks, > Mark > > > > >CC: Mark B Kavanagh <mark.b.kavanagh@intel.com> > >CC: Darrell Ball <dlu998@gmail.com> > >CC: Ciara Loftus <ciara.loftus@intel.com> > >CC: Kevin Traynor <ktraynor@redhat.com> > >CC: Aaron Conole <aconole@redhat.com> > >Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> > >--- > > lib/netdev-dpdk.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >index 7f2d7ed..07c438a 100644 > >--- a/lib/netdev-dpdk.c > >+++ b/lib/netdev-dpdk.c > >@@ -550,6 +550,11 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool > >*mp_exists) > > if (dmp->mp) { > > VLOG_DBG("Allocated \"%s\" mempool with %u mbufs", mp_name, > > dmp->mp_size); > >+ /* rte_pktmbuf_pool_create has done some initialization of the > >+ * rte_mbuf part of each dp_packet. Some OvS specific fields > >+ * of the packet still need to be initialized by > >+ * ovs_rte_pktmbuf_init. */ > >+ rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL); > > } else if (rte_errno == EEXIST) { > > /* A mempool with the same name already exists. We just > > * retrieve its pointer to be returned to the caller. */ > >@@ -566,11 +571,6 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool > >*mp_exists) > > } > > free(mp_name); > > if (dmp->mp) { > >- /* rte_pktmbuf_pool_create has done some initialization of the > >- * rte_mbuf part of each dp_packet, while ovs_rte_pktmbuf_init > >- * initializes some OVS specific fields of dp_packet. > >- */ > >- rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL); > > return dmp; > > } > > } while (!(*mp_exists) && > >-- > >2.4.11
>From: Fischetti, Antonio >Sent: Tuesday, October 17, 2017 6:18 PM >To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@openvswitch.org >Cc: Darrell Ball <dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>; >Kevin Traynor <ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com> >Subject: RE: [PATCH v6 2/5] netdev-dpdk: skip init for existing mempools. > > > >> -----Original Message----- >> From: Kavanagh, Mark B >> Sent: Tuesday, October 17, 2017 2:34 PM >> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org >> Cc: Darrell Ball <dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>; >> Kevin Traynor <ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com> >> Subject: RE: [PATCH v6 2/5] netdev-dpdk: skip init for existing mempools. >> >> >From: Fischetti, Antonio >> >Sent: Monday, October 16, 2017 2:15 PM >> >To: dev@openvswitch.org >> >Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Darrell Ball >> ><dlu998@gmail.com>; Loftus, Ciara <ciara.loftus@intel.com>; Kevin Traynor >> ><ktraynor@redhat.com>; Aaron Conole <aconole@redhat.com>; Fischetti, >Antonio >> ><antonio.fischetti@intel.com> >> >Subject: [PATCH v6 2/5] netdev-dpdk: skip init for existing mempools. >> > >> >Skip initialization of mempool packet areas if this was already >> >done in a previous call to dpdk_mp_create. >> >> Hi Antonio, >> >> As stated in my previous review, I believe that this could probably be >folded >> into patch 1 of the series (it was patch 3 of v5). >> However, I don't object strongly to this patch, so I'll leave it to your >> discretion. > >[Antonio] >I'm keeping this change in a separate patch because it is not related >to the fixes for the mempool management. >It's a small improvement, not so much to save CPU cycles, it's actually >a clean up of the code. Sure thing. In that case, Acked-by: Mark Kavanagh <mark.b.kavanagh@intel.com> Thanks, Mark > >> >> Other than that, LGTM. >> >> Thanks, >> Mark >> >> > >> >CC: Mark B Kavanagh <mark.b.kavanagh@intel.com> >> >CC: Darrell Ball <dlu998@gmail.com> >> >CC: Ciara Loftus <ciara.loftus@intel.com> >> >CC: Kevin Traynor <ktraynor@redhat.com> >> >CC: Aaron Conole <aconole@redhat.com> >> >Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> >> >--- >> > lib/netdev-dpdk.c | 10 +++++----- >> > 1 file changed, 5 insertions(+), 5 deletions(-) >> > >> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> >index 7f2d7ed..07c438a 100644 >> >--- a/lib/netdev-dpdk.c >> >+++ b/lib/netdev-dpdk.c >> >@@ -550,6 +550,11 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool >> >*mp_exists) >> > if (dmp->mp) { >> > VLOG_DBG("Allocated \"%s\" mempool with %u mbufs", mp_name, >> > dmp->mp_size); >> >+ /* rte_pktmbuf_pool_create has done some initialization of the >> >+ * rte_mbuf part of each dp_packet. Some OvS specific fields >> >+ * of the packet still need to be initialized by >> >+ * ovs_rte_pktmbuf_init. */ >> >+ rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL); >> > } else if (rte_errno == EEXIST) { >> > /* A mempool with the same name already exists. We just >> > * retrieve its pointer to be returned to the caller. */ >> >@@ -566,11 +571,6 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool >> >*mp_exists) >> > } >> > free(mp_name); >> > if (dmp->mp) { >> >- /* rte_pktmbuf_pool_create has done some initialization of the >> >- * rte_mbuf part of each dp_packet, while ovs_rte_pktmbuf_init >> >- * initializes some OVS specific fields of dp_packet. >> >- */ >> >- rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL); >> > return dmp; >> > } >> > } while (!(*mp_exists) && >> >-- >> >2.4.11
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 7f2d7ed..07c438a 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -550,6 +550,11 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool *mp_exists) if (dmp->mp) { VLOG_DBG("Allocated \"%s\" mempool with %u mbufs", mp_name, dmp->mp_size); + /* rte_pktmbuf_pool_create has done some initialization of the + * rte_mbuf part of each dp_packet. Some OvS specific fields + * of the packet still need to be initialized by + * ovs_rte_pktmbuf_init. */ + rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL); } else if (rte_errno == EEXIST) { /* A mempool with the same name already exists. We just * retrieve its pointer to be returned to the caller. */ @@ -566,11 +571,6 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool *mp_exists) } free(mp_name); if (dmp->mp) { - /* rte_pktmbuf_pool_create has done some initialization of the - * rte_mbuf part of each dp_packet, while ovs_rte_pktmbuf_init - * initializes some OVS specific fields of dp_packet. - */ - rte_mempool_obj_iter(dmp->mp, ovs_rte_pktmbuf_init, NULL); return dmp; } } while (!(*mp_exists) &&
Skip initialization of mempool packet areas if this was already done in a previous call to dpdk_mp_create. CC: Mark B Kavanagh <mark.b.kavanagh@intel.com> CC: Darrell Ball <dlu998@gmail.com> CC: Ciara Loftus <ciara.loftus@intel.com> CC: Kevin Traynor <ktraynor@redhat.com> CC: Aaron Conole <aconole@redhat.com> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> --- lib/netdev-dpdk.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)