| Message ID | 5642FACF.9030005@studenti.polito.it |
|---|---|
| State | Superseded |
| Headers | show |
> -----Original Message----- > From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Mauricio Vásquez > Sent: Wednesday, November 11, 2015 8:23 AM > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH] netdev-dpdk: Modify rings creation attributes > > Although netdev does explicit locking, it is only valid from the ovs > perspective, then only the ring ends used by ovs should be declared as > single producer / single consumer. > The other ends that are used by the application should be declared as > multiple producer / multiple consumer that is the most general case. LGTM. It would have a slight performance impact where multi producer/consumer is not needed, but on balance gives more flexibility and reduces the chance of a difficult to find bug. > > Please ignore previous patch that was bad-formatted. > (http://openvswitch.org/pipermail/dev/2015-November/062079.html) > > Signed-off-by: Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> > > --- > lib/netdev-dpdk.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 4658416..e3a0771 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1931,9 +1931,9 @@ dpdk_ring_create(const char dev_name[], unsigned int > port_no, > return -err; > } > > - /* Create single consumer/producer rings, netdev does explicit locking. > */ > + /* Create single producer tx ring, netdev does explicit locking. */ > ivshmem->cring_tx = rte_ring_create(ring_name, DPDK_RING_SIZE, SOCKET0, > - RING_F_SP_ENQ | RING_F_SC_DEQ); > + RING_F_SP_ENQ); > if (ivshmem->cring_tx == NULL) { > rte_free(ivshmem); > return ENOMEM; > @@ -1944,9 +1944,9 @@ dpdk_ring_create(const char dev_name[], unsigned int > port_no, > return -err; > } > > - /* Create single consumer/producer rings, netdev does explicit locking. > */ > + /* Create single consumer rx ring, netdev does explicit locking. */ > ivshmem->cring_rx = rte_ring_create(ring_name, DPDK_RING_SIZE, SOCKET0, > - RING_F_SP_ENQ | RING_F_SC_DEQ); > + RING_F_SC_DEQ); > if (ivshmem->cring_rx == NULL) { > rte_free(ivshmem); > return ENOMEM; > -- > 1.9.1 > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Wed, Nov 11, 2015 at 09:22:39AM +0100, Mauricio Vásquez wrote: The commit title can be more clear like "netdev-dpdk: assume dpdr peer can be multi-producer/consumer" > Although netdev does explicit locking, it is only valid from the ovs > perspective, then only the ring ends used by ovs should be declared as > single producer / single consumer. > The other ends that are used by the application should be declared as > multiple producer / multiple consumer that is the most general case. > > Please ignore previous patch that was bad-formatted. > (http://openvswitch.org/pipermail/dev/2015-November/062079.html) We usually do the other way around. We post the new version, then go back to the previous one and reply saying it's fixed on a new version here <url>. Otherwise those lines will be recorded in the commit log which isn't good (add no value) and someone reviewing the old thread might not know about the new version. The patch itself makes sense to me. fbl > Signed-off-by: Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> > > --- > lib/netdev-dpdk.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 4658416..e3a0771 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1931,9 +1931,9 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no, > return -err; > } > > - /* Create single consumer/producer rings, netdev does explicit locking. */ > + /* Create single producer tx ring, netdev does explicit locking. */ > ivshmem->cring_tx = rte_ring_create(ring_name, DPDK_RING_SIZE, SOCKET0, > - RING_F_SP_ENQ | RING_F_SC_DEQ); > + RING_F_SP_ENQ); > if (ivshmem->cring_tx == NULL) { > rte_free(ivshmem); > return ENOMEM; > @@ -1944,9 +1944,9 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no, > return -err; > } > > - /* Create single consumer/producer rings, netdev does explicit locking. */ > + /* Create single consumer rx ring, netdev does explicit locking. */ > ivshmem->cring_rx = rte_ring_create(ring_name, DPDK_RING_SIZE, SOCKET0, > - RING_F_SP_ENQ | RING_F_SC_DEQ); > + RING_F_SC_DEQ); > if (ivshmem->cring_rx == NULL) { > rte_free(ivshmem); > return ENOMEM; > -- > 1.9.1 > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On 16 November 2015 at 06:48, Flavio Leitner <fbl@sysclose.org> wrote: > On Wed, Nov 11, 2015 at 09:22:39AM +0100, Mauricio Vásquez wrote: > > The commit title can be more clear like > "netdev-dpdk: assume dpdr peer can be multi-producer/consumer" > I agree, you just missed a letter, "netdev-dpdk: assume dpdkr peer can be multi-producer/consumer" > > > Although netdev does explicit locking, it is only valid from the ovs > > perspective, then only the ring ends used by ovs should be declared as > > single producer / single consumer. > > The other ends that are used by the application should be declared as > > multiple producer / multiple consumer that is the most general case. > > > > Please ignore previous patch that was bad-formatted. > > (http://openvswitch.org/pipermail/dev/2015-November/062079.html) > > We usually do the other way around. We post the new version, then > go back to the previous one and reply saying it's fixed on a new > version here <url>. Otherwise those lines will be recorded in the > commit log which isn't good (add no value) and someone reviewing > the old thread might not know about the new version. > > Thank you for the advise, I'll do that way next time. > The patch itself makes sense to me. > fbl > > Should I send a new patch with the new commit title and removing the URL in the commit message, or the person that will apply it can change them? > > Signed-off-by: Mauricio Vasquez B < > mauricio.vasquezbernal@studenti.polito.it> > > > > --- > > lib/netdev-dpdk.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index 4658416..e3a0771 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -1931,9 +1931,9 @@ dpdk_ring_create(const char dev_name[], unsigned > int port_no, > > return -err; > > } > > > > - /* Create single consumer/producer rings, netdev does explicit > locking. */ > > + /* Create single producer tx ring, netdev does explicit locking. */ > > ivshmem->cring_tx = rte_ring_create(ring_name, DPDK_RING_SIZE, > SOCKET0, > > - RING_F_SP_ENQ | RING_F_SC_DEQ); > > + RING_F_SP_ENQ); > > if (ivshmem->cring_tx == NULL) { > > rte_free(ivshmem); > > return ENOMEM; > > @@ -1944,9 +1944,9 @@ dpdk_ring_create(const char dev_name[], unsigned > int port_no, > > return -err; > > } > > > > - /* Create single consumer/producer rings, netdev does explicit > locking. */ > > + /* Create single consumer rx ring, netdev does explicit locking. */ > > ivshmem->cring_rx = rte_ring_create(ring_name, DPDK_RING_SIZE, > SOCKET0, > > - RING_F_SP_ENQ | RING_F_SC_DEQ); > > + RING_F_SC_DEQ); > > if (ivshmem->cring_rx == NULL) { > > rte_free(ivshmem); > > return ENOMEM; > > -- > > 1.9.1 > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > >
On Mon, Nov 16, 2015 at 11:25:11AM +0100, Mauricio Vásquez wrote: > On 16 November 2015 at 06:48, Flavio Leitner <fbl@sysclose.org> wrote: > > On Wed, Nov 11, 2015 at 09:22:39AM +0100, Mauricio Vásquez wrote: > > > Although netdev does explicit locking, it is only valid from the ovs > > > perspective, then only the ring ends used by ovs should be declared as > > > single producer / single consumer. > > > The other ends that are used by the application should be declared as > > > multiple producer / multiple consumer that is the most general case. > > > > > > Please ignore previous patch that was bad-formatted. > > > (http://openvswitch.org/pipermail/dev/2015-November/062079.html) > > > > We usually do the other way around. We post the new version, then > > go back to the previous one and reply saying it's fixed on a new > > version here <url>. Otherwise those lines will be recorded in the > > commit log which isn't good (add no value) and someone reviewing > > the old thread might not know about the new version. > > > > Thank you for the advise, I'll do that way next time. > > > The patch itself makes sense to me. > > fbl > > > > Should I send a new patch with the new commit title and > removing the URL in the commit message, or the person that > will apply it can change them? Maintainers have to deal with many patches plus reviews, so we appreciate if you can post another version. Doing so will help us to ack your patch properly and to get it merged smoothly. fbl
I have sent a new patch modifying the commit title and commit message: http://openvswitch.org/pipermail/dev/2015-November/062257.html Mauricio V, On 16 November 2015 at 19:39, Flavio Leitner <fbl@sysclose.org> wrote: > On Mon, Nov 16, 2015 at 11:25:11AM +0100, Mauricio Vásquez wrote: > > On 16 November 2015 at 06:48, Flavio Leitner <fbl@sysclose.org> wrote: > > > On Wed, Nov 11, 2015 at 09:22:39AM +0100, Mauricio Vásquez wrote: > > > > Although netdev does explicit locking, it is only valid from the ovs > > > > perspective, then only the ring ends used by ovs should be declared > as > > > > single producer / single consumer. > > > > The other ends that are used by the application should be declared as > > > > multiple producer / multiple consumer that is the most general case. > > > > > > > > Please ignore previous patch that was bad-formatted. > > > > (http://openvswitch.org/pipermail/dev/2015-November/062079.html) > > > > > > We usually do the other way around. We post the new version, then > > > go back to the previous one and reply saying it's fixed on a new > > > version here <url>. Otherwise those lines will be recorded in the > > > commit log which isn't good (add no value) and someone reviewing > > > the old thread might not know about the new version. > > > > > > Thank you for the advise, I'll do that way next time. > > > > > The patch itself makes sense to me. > > > fbl > > > > > > Should I send a new patch with the new commit title and > > removing the URL in the commit message, or the person that > > will apply it can change them? > > Maintainers have to deal with many patches plus reviews, so we > appreciate if you can post another version. Doing so will help > us to ack your patch properly and to get it merged smoothly. > > fbl > > >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 4658416..e3a0771 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1931,9 +1931,9 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no, return -err; } - /* Create single consumer/producer rings, netdev does explicit locking. */ + /* Create single producer tx ring, netdev does explicit locking. */ ivshmem->cring_tx = rte_ring_create(ring_name, DPDK_RING_SIZE, SOCKET0, - RING_F_SP_ENQ | RING_F_SC_DEQ); + RING_F_SP_ENQ); if (ivshmem->cring_tx == NULL) { rte_free(ivshmem); return ENOMEM; @@ -1944,9 +1944,9 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no, return -err; } - /* Create single consumer/producer rings, netdev does explicit locking. */ + /* Create single consumer rx ring, netdev does explicit locking. */ ivshmem->cring_rx = rte_ring_create(ring_name, DPDK_RING_SIZE, SOCKET0, - RING_F_SP_ENQ | RING_F_SC_DEQ); + RING_F_SC_DEQ); if (ivshmem->cring_rx == NULL) { rte_free(ivshmem); return ENOMEM;
Although netdev does explicit locking, it is only valid from the ovs perspective, then only the ring ends used by ovs should be declared as single producer / single consumer. The other ends that are used by the application should be declared as multiple producer / multiple consumer that is the most general case. Please ignore previous patch that was bad-formatted. (http://openvswitch.org/pipermail/dev/2015-November/062079.html) Signed-off-by: Mauricio Vasquez B <mauricio.vasquezbernal@studenti.polito.it> --- lib/netdev-dpdk.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)