* [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies @ 2015-03-25 18:43 Zoltan Kiss 2015-03-26 1:20 ` Ananyev, Konstantin 0 siblings, 1 reply; 6+ messages in thread From: Zoltan Kiss @ 2015-03-25 18:43 UTC (permalink / raw) To: dev; +Cc: Zoltan Kiss This macro and function were copies from the mbuf library, no reason to keep them. Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> --- examples/vhost/main.c | 38 +++++--------------------------------- 1 file changed, 5 insertions(+), 33 deletions(-) diff --git a/examples/vhost/main.c b/examples/vhost/main.c index c3fcb80..1c998a5 100644 --- a/examples/vhost/main.c +++ b/examples/vhost/main.c @@ -139,8 +139,6 @@ /* Number of descriptors per cacheline. */ #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc)) -#define MBUF_EXT_MEM(mb) (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb)) - /* mask of enabled ports */ static uint32_t enabled_port_mask = 0; @@ -1538,32 +1536,6 @@ attach_rxmbuf_zcp(struct virtio_net *dev) return; } -/* - * Detach an attched packet mbuf - - * - restore original mbuf address and length values. - * - reset pktmbuf data and data_len to their default values. - * All other fields of the given packet mbuf will be left intact. - * - * @param m - * The attached packet mbuf. - */ -static inline void pktmbuf_detach_zcp(struct rte_mbuf *m) -{ - const struct rte_mempool *mp = m->pool; - void *buf = RTE_MBUF_TO_BADDR(m); - uint32_t buf_ofs; - uint32_t buf_len = mp->elt_size - sizeof(*m); - m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m); - - m->buf_addr = buf; - m->buf_len = (uint16_t)buf_len; - - buf_ofs = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ? - RTE_PKTMBUF_HEADROOM : m->buf_len; - m->data_off = buf_ofs; - - m->data_len = 0; -} /* * This function is called after packets have been transimited. It fetchs mbuf @@ -1590,8 +1562,8 @@ txmbuf_clean_zcp(struct virtio_net *dev, struct vpool *vpool) for (index = 0; index < mbuf_count; index++) { mbuf = __rte_mbuf_raw_alloc(vpool->pool); - if (likely(MBUF_EXT_MEM(mbuf))) - pktmbuf_detach_zcp(mbuf); + if (likely(RTE_MBUF_INDIRECT(mbuf))) + rte_pktmbuf_detach(mbuf); rte_ring_sp_enqueue(vpool->ring, mbuf); /* Update used index buffer information. */ @@ -1653,8 +1625,8 @@ static void mbuf_destroy_zcp(struct vpool *vpool) for (index = 0; index < mbuf_count; index++) { mbuf = __rte_mbuf_raw_alloc(vpool->pool); if (likely(mbuf != NULL)) { - if (likely(MBUF_EXT_MEM(mbuf))) - pktmbuf_detach_zcp(mbuf); + if (likely(RTE_MBUF_INDIRECT(mbuf))) + rte_pktmbuf_detach(mbuf); rte_ring_sp_enqueue(vpool->ring, (void *)mbuf); } } @@ -2149,7 +2121,7 @@ switch_worker_zcp(__attribute__((unused)) void *arg) } while (likely(rx_count)) { rx_count--; - pktmbuf_detach_zcp( + rte_pktmbuf_detach( pkts_burst[rx_count]); rte_ring_sp_enqueue( vpool_array[index].ring, -- 1.9.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies 2015-03-25 18:43 [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies Zoltan Kiss @ 2015-03-26 1:20 ` Ananyev, Konstantin 2015-03-26 3:32 ` Ouyang, Changchun 2015-03-26 16:46 ` Zoltan Kiss 0 siblings, 2 replies; 6+ messages in thread From: Ananyev, Konstantin @ 2015-03-26 1:20 UTC (permalink / raw) To: Zoltan Kiss, dev > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss > Sent: Wednesday, March 25, 2015 6:43 PM > To: dev@dpdk.org > Cc: Zoltan Kiss > Subject: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies > > This macro and function were copies from the mbuf library, no reason to keep > them. NACK You can't use RTE_MBUF_INDIRECT macro here. If you'll look at vhost code carefully, you'll realise that we don't use standard rte_pktmbuf_attach() here. As we attach mbuf not to another mbuf but to external memory buffer, passed to us by virtio device. Look at attach_rxmbuf_zcp(). Though I suppose, we can replace pktmbuf_detach_zcp() , with rte_pktmbuf_detach() - they are doing identical things. BTW, I wonder did you ever test your patch? My guess it would cause vhost with '--zero-copy' to crash or corrupt the packets straightway. Konstantin > > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> > --- > examples/vhost/main.c | 38 +++++--------------------------------- > 1 file changed, 5 insertions(+), 33 deletions(-) > > diff --git a/examples/vhost/main.c b/examples/vhost/main.c > index c3fcb80..1c998a5 100644 > --- a/examples/vhost/main.c > +++ b/examples/vhost/main.c > @@ -139,8 +139,6 @@ > /* Number of descriptors per cacheline. */ > #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc)) > > -#define MBUF_EXT_MEM(mb) (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb)) > - > /* mask of enabled ports */ > static uint32_t enabled_port_mask = 0; > > @@ -1538,32 +1536,6 @@ attach_rxmbuf_zcp(struct virtio_net *dev) > return; > } > > -/* > - * Detach an attched packet mbuf - > - * - restore original mbuf address and length values. > - * - reset pktmbuf data and data_len to their default values. > - * All other fields of the given packet mbuf will be left intact. > - * > - * @param m > - * The attached packet mbuf. > - */ > -static inline void pktmbuf_detach_zcp(struct rte_mbuf *m) > -{ > - const struct rte_mempool *mp = m->pool; > - void *buf = RTE_MBUF_TO_BADDR(m); > - uint32_t buf_ofs; > - uint32_t buf_len = mp->elt_size - sizeof(*m); > - m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m); > - > - m->buf_addr = buf; > - m->buf_len = (uint16_t)buf_len; > - > - buf_ofs = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ? > - RTE_PKTMBUF_HEADROOM : m->buf_len; > - m->data_off = buf_ofs; > - > - m->data_len = 0; > -} > > /* > * This function is called after packets have been transimited. It fetchs mbuf > @@ -1590,8 +1562,8 @@ txmbuf_clean_zcp(struct virtio_net *dev, struct vpool *vpool) > > for (index = 0; index < mbuf_count; index++) { > mbuf = __rte_mbuf_raw_alloc(vpool->pool); > - if (likely(MBUF_EXT_MEM(mbuf))) > - pktmbuf_detach_zcp(mbuf); > + if (likely(RTE_MBUF_INDIRECT(mbuf))) > + rte_pktmbuf_detach(mbuf); > rte_ring_sp_enqueue(vpool->ring, mbuf); > > /* Update used index buffer information. */ > @@ -1653,8 +1625,8 @@ static void mbuf_destroy_zcp(struct vpool *vpool) > for (index = 0; index < mbuf_count; index++) { > mbuf = __rte_mbuf_raw_alloc(vpool->pool); > if (likely(mbuf != NULL)) { > - if (likely(MBUF_EXT_MEM(mbuf))) > - pktmbuf_detach_zcp(mbuf); > + if (likely(RTE_MBUF_INDIRECT(mbuf))) > + rte_pktmbuf_detach(mbuf); > rte_ring_sp_enqueue(vpool->ring, (void *)mbuf); > } > } > @@ -2149,7 +2121,7 @@ switch_worker_zcp(__attribute__((unused)) void *arg) > } > while (likely(rx_count)) { > rx_count--; > - pktmbuf_detach_zcp( > + rte_pktmbuf_detach( > pkts_burst[rx_count]); > rte_ring_sp_enqueue( > vpool_array[index].ring, > -- > 1.9.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies 2015-03-26 1:20 ` Ananyev, Konstantin @ 2015-03-26 3:32 ` Ouyang, Changchun 2015-03-26 16:46 ` Zoltan Kiss 1 sibling, 0 replies; 6+ messages in thread From: Ouyang, Changchun @ 2015-03-26 3:32 UTC (permalink / raw) To: Ananyev, Konstantin, Zoltan Kiss, dev > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, > Konstantin > Sent: Thursday, March 26, 2015 9:21 AM > To: Zoltan Kiss; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] examples/vhost: use library routines > instead of local copies > > > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss > > Sent: Wednesday, March 25, 2015 6:43 PM > > To: dev@dpdk.org > > Cc: Zoltan Kiss > > Subject: [dpdk-dev] [PATCH] examples/vhost: use library routines > > instead of local copies > > > > This macro and function were copies from the mbuf library, no reason > > to keep them. > > NACK > You can't use RTE_MBUF_INDIRECT macro here. > If you'll look at vhost code carefully, you'll realise that we don't use standard > rte_pktmbuf_attach() here. > As we attach mbuf not to another mbuf but to external memory buffer, > passed to us by virtio device. > Look at attach_rxmbuf_zcp(). > Though I suppose, we can replace pktmbuf_detach_zcp() , with > rte_pktmbuf_detach() - they are doing identical things. > BTW, I wonder did you ever test your patch? > My guess it would cause vhost with '--zero-copy' to crash or corrupt the > packets straightway. > > Konstantin > > > > > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> NACK this patch, Same reason with Konstantin. Changchun ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies 2015-03-26 1:20 ` Ananyev, Konstantin 2015-03-26 3:32 ` Ouyang, Changchun @ 2015-03-26 16:46 ` Zoltan Kiss 2015-03-26 17:34 ` Ananyev, Konstantin 1 sibling, 1 reply; 6+ messages in thread From: Zoltan Kiss @ 2015-03-26 16:46 UTC (permalink / raw) To: Ananyev, Konstantin, dev On 26/03/15 01:20, Ananyev, Konstantin wrote: > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss >> Sent: Wednesday, March 25, 2015 6:43 PM >> To: dev@dpdk.org >> Cc: Zoltan Kiss >> Subject: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies >> >> This macro and function were copies from the mbuf library, no reason to keep >> them. > > NACK > You can't use RTE_MBUF_INDIRECT macro here. > If you'll look at vhost code carefully, you'll realise that we don't use standard rte_pktmbuf_attach() here. > As we attach mbuf not to another mbuf but to external memory buffer, passed to us by virtio device. > Look at attach_rxmbuf_zcp(). Yes, I think the proper fix is to set the flag in attach_rxmbuf_zcp() and virtio_tx_route_zcp(), then you can use the library macro here. > Though I suppose, we can replace pktmbuf_detach_zcp() , with rte_pktmbuf_detach() - they are doing identical things. Yes, the only difference is that the latter do "m->ol_flags = 0" as well. > BTW, I wonder did you ever test your patch? Indeed I did not, shame on me. I don't have a KVM setup at hand. This fix were born as a side effect of the cleaning up in the library, and I'm afraid I don't have the time right now to create a KVM setup. Could anyone who has it at hand help out to run a quick test? (for the v2 of this patch, which I'll send in shortly) Regards, Zoltan > My guess it would cause vhost with '--zero-copy' to crash or corrupt the packets straightway. > > Konstantin > >> >> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >> --- >> examples/vhost/main.c | 38 +++++--------------------------------- >> 1 file changed, 5 insertions(+), 33 deletions(-) >> >> diff --git a/examples/vhost/main.c b/examples/vhost/main.c >> index c3fcb80..1c998a5 100644 >> --- a/examples/vhost/main.c >> +++ b/examples/vhost/main.c >> @@ -139,8 +139,6 @@ >> /* Number of descriptors per cacheline. */ >> #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc)) >> >> -#define MBUF_EXT_MEM(mb) (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb)) >> - >> /* mask of enabled ports */ >> static uint32_t enabled_port_mask = 0; >> >> @@ -1538,32 +1536,6 @@ attach_rxmbuf_zcp(struct virtio_net *dev) >> return; >> } >> >> -/* >> - * Detach an attched packet mbuf - >> - * - restore original mbuf address and length values. >> - * - reset pktmbuf data and data_len to their default values. >> - * All other fields of the given packet mbuf will be left intact. >> - * >> - * @param m >> - * The attached packet mbuf. >> - */ >> -static inline void pktmbuf_detach_zcp(struct rte_mbuf *m) >> -{ >> - const struct rte_mempool *mp = m->pool; >> - void *buf = RTE_MBUF_TO_BADDR(m); >> - uint32_t buf_ofs; >> - uint32_t buf_len = mp->elt_size - sizeof(*m); >> - m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m); >> - >> - m->buf_addr = buf; >> - m->buf_len = (uint16_t)buf_len; >> - >> - buf_ofs = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ? >> - RTE_PKTMBUF_HEADROOM : m->buf_len; >> - m->data_off = buf_ofs; >> - >> - m->data_len = 0; >> -} >> >> /* >> * This function is called after packets have been transimited. It fetchs mbuf >> @@ -1590,8 +1562,8 @@ txmbuf_clean_zcp(struct virtio_net *dev, struct vpool *vpool) >> >> for (index = 0; index < mbuf_count; index++) { >> mbuf = __rte_mbuf_raw_alloc(vpool->pool); >> - if (likely(MBUF_EXT_MEM(mbuf))) >> - pktmbuf_detach_zcp(mbuf); >> + if (likely(RTE_MBUF_INDIRECT(mbuf))) >> + rte_pktmbuf_detach(mbuf); >> rte_ring_sp_enqueue(vpool->ring, mbuf); >> >> /* Update used index buffer information. */ >> @@ -1653,8 +1625,8 @@ static void mbuf_destroy_zcp(struct vpool *vpool) >> for (index = 0; index < mbuf_count; index++) { >> mbuf = __rte_mbuf_raw_alloc(vpool->pool); >> if (likely(mbuf != NULL)) { >> - if (likely(MBUF_EXT_MEM(mbuf))) >> - pktmbuf_detach_zcp(mbuf); >> + if (likely(RTE_MBUF_INDIRECT(mbuf))) >> + rte_pktmbuf_detach(mbuf); >> rte_ring_sp_enqueue(vpool->ring, (void *)mbuf); >> } >> } >> @@ -2149,7 +2121,7 @@ switch_worker_zcp(__attribute__((unused)) void *arg) >> } >> while (likely(rx_count)) { >> rx_count--; >> - pktmbuf_detach_zcp( >> + rte_pktmbuf_detach( >> pkts_burst[rx_count]); >> rte_ring_sp_enqueue( >> vpool_array[index].ring, >> -- >> 1.9.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies 2015-03-26 16:46 ` Zoltan Kiss @ 2015-03-26 17:34 ` Ananyev, Konstantin 2015-03-26 18:01 ` Zoltan Kiss 0 siblings, 1 reply; 6+ messages in thread From: Ananyev, Konstantin @ 2015-03-26 17:34 UTC (permalink / raw) To: Zoltan Kiss, dev > -----Original Message----- > From: Zoltan Kiss [mailto:zoltan.kiss@linaro.org] > Sent: Thursday, March 26, 2015 4:46 PM > To: Ananyev, Konstantin; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies > > > > On 26/03/15 01:20, Ananyev, Konstantin wrote: > > > > > >> -----Original Message----- > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss > >> Sent: Wednesday, March 25, 2015 6:43 PM > >> To: dev@dpdk.org > >> Cc: Zoltan Kiss > >> Subject: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies > >> > >> This macro and function were copies from the mbuf library, no reason to keep > >> them. > > > > NACK > > You can't use RTE_MBUF_INDIRECT macro here. > > If you'll look at vhost code carefully, you'll realise that we don't use standard rte_pktmbuf_attach() here. > > As we attach mbuf not to another mbuf but to external memory buffer, passed to us by virtio device. > > Look at attach_rxmbuf_zcp(). > Yes, I think the proper fix is to set the flag in attach_rxmbuf_zcp() > and virtio_tx_route_zcp(), then you can use the library macro here. No, it is not. IND_ATTACHED_MBUF flag indicates that that mbuf attached to another mbuf and __rte_pktmbuf_prefree_seg() would try to do mbuf detach. We definetly don't want to set IND_ATTACHED_MBUF here. I think there is no need to fix anything here. Konstantin > > > Though I suppose, we can replace pktmbuf_detach_zcp() , with rte_pktmbuf_detach() - they are doing identical things. > Yes, the only difference is that the latter do "m->ol_flags = 0" as well. > > > BTW, I wonder did you ever test your patch? > Indeed I did not, shame on me. I don't have a KVM setup at hand. This > fix were born as a side effect of the cleaning up in the library, > and I'm afraid I don't have the time right now to create a KVM setup. > Could anyone who has it at hand help out to run a quick test? (for the > v2 of this patch, which I'll send in shortly) > > Regards, > > Zoltan > > > My guess it would cause vhost with '--zero-copy' to crash or corrupt the packets straightway. > > > > Konstantin > > > >> > >> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> > >> --- > >> examples/vhost/main.c | 38 +++++--------------------------------- > >> 1 file changed, 5 insertions(+), 33 deletions(-) > >> > >> diff --git a/examples/vhost/main.c b/examples/vhost/main.c > >> index c3fcb80..1c998a5 100644 > >> --- a/examples/vhost/main.c > >> +++ b/examples/vhost/main.c > >> @@ -139,8 +139,6 @@ > >> /* Number of descriptors per cacheline. */ > >> #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc)) > >> > >> -#define MBUF_EXT_MEM(mb) (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb)) > >> - > >> /* mask of enabled ports */ > >> static uint32_t enabled_port_mask = 0; > >> > >> @@ -1538,32 +1536,6 @@ attach_rxmbuf_zcp(struct virtio_net *dev) > >> return; > >> } > >> > >> -/* > >> - * Detach an attched packet mbuf - > >> - * - restore original mbuf address and length values. > >> - * - reset pktmbuf data and data_len to their default values. > >> - * All other fields of the given packet mbuf will be left intact. > >> - * > >> - * @param m > >> - * The attached packet mbuf. > >> - */ > >> -static inline void pktmbuf_detach_zcp(struct rte_mbuf *m) > >> -{ > >> - const struct rte_mempool *mp = m->pool; > >> - void *buf = RTE_MBUF_TO_BADDR(m); > >> - uint32_t buf_ofs; > >> - uint32_t buf_len = mp->elt_size - sizeof(*m); > >> - m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m); > >> - > >> - m->buf_addr = buf; > >> - m->buf_len = (uint16_t)buf_len; > >> - > >> - buf_ofs = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ? > >> - RTE_PKTMBUF_HEADROOM : m->buf_len; > >> - m->data_off = buf_ofs; > >> - > >> - m->data_len = 0; > >> -} > >> > >> /* > >> * This function is called after packets have been transimited. It fetchs mbuf > >> @@ -1590,8 +1562,8 @@ txmbuf_clean_zcp(struct virtio_net *dev, struct vpool *vpool) > >> > >> for (index = 0; index < mbuf_count; index++) { > >> mbuf = __rte_mbuf_raw_alloc(vpool->pool); > >> - if (likely(MBUF_EXT_MEM(mbuf))) > >> - pktmbuf_detach_zcp(mbuf); > >> + if (likely(RTE_MBUF_INDIRECT(mbuf))) > >> + rte_pktmbuf_detach(mbuf); > >> rte_ring_sp_enqueue(vpool->ring, mbuf); > >> > >> /* Update used index buffer information. */ > >> @@ -1653,8 +1625,8 @@ static void mbuf_destroy_zcp(struct vpool *vpool) > >> for (index = 0; index < mbuf_count; index++) { > >> mbuf = __rte_mbuf_raw_alloc(vpool->pool); > >> if (likely(mbuf != NULL)) { > >> - if (likely(MBUF_EXT_MEM(mbuf))) > >> - pktmbuf_detach_zcp(mbuf); > >> + if (likely(RTE_MBUF_INDIRECT(mbuf))) > >> + rte_pktmbuf_detach(mbuf); > >> rte_ring_sp_enqueue(vpool->ring, (void *)mbuf); > >> } > >> } > >> @@ -2149,7 +2121,7 @@ switch_worker_zcp(__attribute__((unused)) void *arg) > >> } > >> while (likely(rx_count)) { > >> rx_count--; > >> - pktmbuf_detach_zcp( > >> + rte_pktmbuf_detach( > >> pkts_burst[rx_count]); > >> rte_ring_sp_enqueue( > >> vpool_array[index].ring, > >> -- > >> 1.9.1 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies 2015-03-26 17:34 ` Ananyev, Konstantin @ 2015-03-26 18:01 ` Zoltan Kiss 0 siblings, 0 replies; 6+ messages in thread From: Zoltan Kiss @ 2015-03-26 18:01 UTC (permalink / raw) To: Ananyev, Konstantin, dev On 26/03/15 17:34, Ananyev, Konstantin wrote: > > >> -----Original Message----- >> From: Zoltan Kiss [mailto:zoltan.kiss@linaro.org] >> Sent: Thursday, March 26, 2015 4:46 PM >> To: Ananyev, Konstantin; dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies >> >> >> >> On 26/03/15 01:20, Ananyev, Konstantin wrote: >>> >>> >>>> -----Original Message----- >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss >>>> Sent: Wednesday, March 25, 2015 6:43 PM >>>> To: dev@dpdk.org >>>> Cc: Zoltan Kiss >>>> Subject: [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies >>>> >>>> This macro and function were copies from the mbuf library, no reason to keep >>>> them. >>> >>> NACK >>> You can't use RTE_MBUF_INDIRECT macro here. >>> If you'll look at vhost code carefully, you'll realise that we don't use standard rte_pktmbuf_attach() here. >>> As we attach mbuf not to another mbuf but to external memory buffer, passed to us by virtio device. >>> Look at attach_rxmbuf_zcp(). >> Yes, I think the proper fix is to set the flag in attach_rxmbuf_zcp() >> and virtio_tx_route_zcp(), then you can use the library macro here. > > No, it is not. > IND_ATTACHED_MBUF flag indicates that that mbuf attached to another mbuf and __rte_pktmbuf_prefree_seg() > would try to do mbuf detach. > We definetly don't want to set IND_ATTACHED_MBUF here. I see. Quite confusing how vhost reuse some library code to do something slightly different. > I think there is no need to fix anything here. > > Konstantin > >> >>> Though I suppose, we can replace pktmbuf_detach_zcp() , with rte_pktmbuf_detach() - they are doing identical things. >> Yes, the only difference is that the latter do "m->ol_flags = 0" as well. >> >>> BTW, I wonder did you ever test your patch? >> Indeed I did not, shame on me. I don't have a KVM setup at hand. This >> fix were born as a side effect of the cleaning up in the library, >> and I'm afraid I don't have the time right now to create a KVM setup. >> Could anyone who has it at hand help out to run a quick test? (for the >> v2 of this patch, which I'll send in shortly) > > >> >> Regards, >> >> Zoltan >> >>> My guess it would cause vhost with '--zero-copy' to crash or corrupt the packets straightway. >>> >>> Konstantin >>> >>>> >>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >>>> --- >>>> examples/vhost/main.c | 38 +++++--------------------------------- >>>> 1 file changed, 5 insertions(+), 33 deletions(-) >>>> >>>> diff --git a/examples/vhost/main.c b/examples/vhost/main.c >>>> index c3fcb80..1c998a5 100644 >>>> --- a/examples/vhost/main.c >>>> +++ b/examples/vhost/main.c >>>> @@ -139,8 +139,6 @@ >>>> /* Number of descriptors per cacheline. */ >>>> #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc)) >>>> >>>> -#define MBUF_EXT_MEM(mb) (RTE_MBUF_FROM_BADDR((mb)->buf_addr) != (mb)) >>>> - >>>> /* mask of enabled ports */ >>>> static uint32_t enabled_port_mask = 0; >>>> >>>> @@ -1538,32 +1536,6 @@ attach_rxmbuf_zcp(struct virtio_net *dev) >>>> return; >>>> } >>>> >>>> -/* >>>> - * Detach an attched packet mbuf - >>>> - * - restore original mbuf address and length values. >>>> - * - reset pktmbuf data and data_len to their default values. >>>> - * All other fields of the given packet mbuf will be left intact. >>>> - * >>>> - * @param m >>>> - * The attached packet mbuf. >>>> - */ >>>> -static inline void pktmbuf_detach_zcp(struct rte_mbuf *m) >>>> -{ >>>> - const struct rte_mempool *mp = m->pool; >>>> - void *buf = RTE_MBUF_TO_BADDR(m); >>>> - uint32_t buf_ofs; >>>> - uint32_t buf_len = mp->elt_size - sizeof(*m); >>>> - m->buf_physaddr = rte_mempool_virt2phy(mp, m) + sizeof(*m); >>>> - >>>> - m->buf_addr = buf; >>>> - m->buf_len = (uint16_t)buf_len; >>>> - >>>> - buf_ofs = (RTE_PKTMBUF_HEADROOM <= m->buf_len) ? >>>> - RTE_PKTMBUF_HEADROOM : m->buf_len; >>>> - m->data_off = buf_ofs; >>>> - >>>> - m->data_len = 0; >>>> -} >>>> >>>> /* >>>> * This function is called after packets have been transimited. It fetchs mbuf >>>> @@ -1590,8 +1562,8 @@ txmbuf_clean_zcp(struct virtio_net *dev, struct vpool *vpool) >>>> >>>> for (index = 0; index < mbuf_count; index++) { >>>> mbuf = __rte_mbuf_raw_alloc(vpool->pool); >>>> - if (likely(MBUF_EXT_MEM(mbuf))) >>>> - pktmbuf_detach_zcp(mbuf); >>>> + if (likely(RTE_MBUF_INDIRECT(mbuf))) >>>> + rte_pktmbuf_detach(mbuf); >>>> rte_ring_sp_enqueue(vpool->ring, mbuf); >>>> >>>> /* Update used index buffer information. */ >>>> @@ -1653,8 +1625,8 @@ static void mbuf_destroy_zcp(struct vpool *vpool) >>>> for (index = 0; index < mbuf_count; index++) { >>>> mbuf = __rte_mbuf_raw_alloc(vpool->pool); >>>> if (likely(mbuf != NULL)) { >>>> - if (likely(MBUF_EXT_MEM(mbuf))) >>>> - pktmbuf_detach_zcp(mbuf); >>>> + if (likely(RTE_MBUF_INDIRECT(mbuf))) >>>> + rte_pktmbuf_detach(mbuf); >>>> rte_ring_sp_enqueue(vpool->ring, (void *)mbuf); >>>> } >>>> } >>>> @@ -2149,7 +2121,7 @@ switch_worker_zcp(__attribute__((unused)) void *arg) >>>> } >>>> while (likely(rx_count)) { >>>> rx_count--; >>>> - pktmbuf_detach_zcp( >>>> + rte_pktmbuf_detach( >>>> pkts_burst[rx_count]); >>>> rte_ring_sp_enqueue( >>>> vpool_array[index].ring, >>>> -- >>>> 1.9.1 >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-26 18:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-25 18:43 [dpdk-dev] [PATCH] examples/vhost: use library routines instead of local copies Zoltan Kiss 2015-03-26 1:20 ` Ananyev, Konstantin 2015-03-26 3:32 ` Ouyang, Changchun 2015-03-26 16:46 ` Zoltan Kiss 2015-03-26 17:34 ` Ananyev, Konstantin 2015-03-26 18:01 ` Zoltan Kiss
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).