[dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf change
2014-09-30 06:43, Xie, Huawei: > > 2014-09-30 02:41, Xie, Huawei: > > > I would rework the patch according to your comment. > > > I don't get clear about this comment. Do you mean that recreate the patch > > > set > > > based on the example that already has this mbuf change? > > > > Yes > > > > > Some of the background you might not know: > > > I fully understand your concern here to make it a better patch and I > > > fully agree > > > with you total comments. > > > This is really a special case. You know it is transform of thousand lines > > > of code > > with modifications. > > > Sometimes a simple change could take me more than one day to rework the > > patch, lines of lines manual check. > > > I have already spent more than one week of time merely on the patch > > > format > > itself. :(. > > > > > I know. I think you are learning (the hard way) how to use git. > > As Ouyang said in this thread, you should use "git rebase" > > and especially the --interactive mode to update your changes. > > And you should make small commits at first. It's easier to squash > > commits than splitting them. > > > > > Could we possibly treat it specially when we have comment whether the > > > patch > > can be split/merged better? > > > > I thought it many times because I see it causes you many troubles. > > But I still think that vhost is an important feature and we probably > > want to be able to understand what are the reasons behond the changes > > by looking at the git history. That's why I'd like you to make smaller > > refactoring commits with explanations in commit logs. > > > > That's said, we should continue working together on it. > > Send me your drafts and I'll help you to split them. The part I cannot do > > by myself is about the explanations in commit logs. > > > Based on the principle "small commit", this is the rough idea. Btw, git tool > will n't help here. Please have a quick read through. I will start the rework > asap. > Patch 1: > copy examples/vhost/main.c/lib/librte_vhost/vhost/vhost_rxtx.c > git mv examples/vhost/eventfd_link /lib/librte_vhost/ > git mv examples/vhost/libvirt /lib/librte/vhost/libvirt > git mv examples/vhost/vhost-net-cdev.c /lib/librte_vhost/ > git mv examples/vhost/vhost-net-cdev.h /lib/librte_vhost/ > git mv examples/vhost/virtio-net.* /lib/librte_vhost/ > comment: > As in previous patch set, vhost_rxtx.c is partly copied from main.c, here I > decide to "copy" rather than "mv" main.c from example to vhost lib. The > drawback is gitk couldn't recognize main.c and vhost_rxtx.c have the same > index. > The pros is even with mv, gitk couldn't recognize partly copy of > vhost_rxtx.c, and later we could patch vhost example based on existing files. > Will emphasize that vhost_rxtx.c is a purely copy without any modification in > commit message. > delete examples/vhost/Makefile as vhost example could no longer be compiled > from here after until the example patch. > > Another option is leave all example files, and copy needed ones to vhost lib > directory. The cons is gitk will treat all files in vhost lib as new files. No please move the files to the lib. It's easier to read. You can drop all the example in 1 patch or make as you did before: drop old example and rework it in subsequent patches. The most important part is the library. > patch 2: > rename virtio-net.c to rte_virtio_net.h It can be done in patch 1. > patch 3: > remove zero copy logic in related files. > patch 4: > remove switching related logics in related files. > patch 5: > delete all functions in vhost_rxtx.c except four functions > virtio_dev_(merge)_rx/tx and helper functions copy_mbuf_to_rings.. > comment here: > here virtio_dev_(merge)_rx/tx will refer non-existing functions like > virtio_tx_route. > I think it is ok, right? > patch 6: > remove virtio_dev_tx, and rename virtio_dev_merge_tx to > virtio_dev_tx. > patch virtio_dev_rx, virtio_dev_merge_rx and virtio_dev_tx > will see if this can be further divided. Yes please try to split and explain these important things. > patch 7: > Other minior fixes, like change global vars to static vars > patch 8: > fixes plenty of serious coding style issues > Patch 9: > Vhost API patch > Patch 10: > Added identified TODO or FIXME > patch 11: > Add vhost lib makefiles. > Patch 12...: > Vhost example patch OK for the other ones. The global idea is to isolate minor changes in order to make important changes clearly visible in dedicated patches. -- Thomas
[dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf change
2014-09-30 02:41, Xie, Huawei: > I would rework the patch according to your comment. > I don't get clear about this comment. Do you mean that recreate the patch set > based on the example that already has this mbuf change? Yes > Some of the background you might not know: > I fully understand your concern here to make it a better patch and I fully > agree > with you total comments. > This is really a special case. You know it is transform of thousand lines of > code with modifications. > Sometimes a simple change could take me more than one day to rework the > patch, lines of lines manual check. > I have already spent more than one week of time merely on the patch format > itself. :(. I know. I think you are learning (the hard way) how to use git. As Ouyang said in this thread, you should use "git rebase" and especially the --interactive mode to update your changes. And you should make small commits at first. It's easier to squash commits than splitting them. > Could we possibly treat it specially when we have comment whether the patch > can be split/merged better? I thought it many times because I see it causes you many troubles. But I still think that vhost is an important feature and we probably want to be able to understand what are the reasons behond the changes by looking at the git history. That's why I'd like you to make smaller refactoring commits with explanations in commit logs. That's said, we should continue working together on it. Send me your drafts and I'll help you to split them. The part I cannot do by myself is about the explanations in commit logs. Thanks -- Thomas
[dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf change
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Tuesday, September 30, 2014 12:46 PM > To: Xie, Huawei > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf > change > > 2014-09-30 02:41, Xie, Huawei: > > I would rework the patch according to your comment. > > I don't get clear about this comment. Do you mean that recreate the patch > > set > > based on the example that already has this mbuf change? > > Yes > > > Some of the background you might not know: > > I fully understand your concern here to make it a better patch and I fully > > agree > > with you total comments. > > This is really a special case. You know it is transform of thousand lines > > of code > with modifications. > > Sometimes a simple change could take me more than one day to rework the > patch, lines of lines manual check. > > I have already spent more than one week of time merely on the patch format > itself. :(. > > I know. I think you are learning (the hard way) how to use git. > As Ouyang said in this thread, you should use "git rebase" > and especially the --interactive mode to update your changes. > And you should make small commits at first. It's easier to squash > commits than splitting them. > > > Could we possibly treat it specially when we have comment whether the patch > can be split/merged better? > > I thought it many times because I see it causes you many troubles. > But I still think that vhost is an important feature and we probably > want to be able to understand what are the reasons behond the changes > by looking at the git history. That's why I'd like you to make smaller > refactoring commits with explanations in commit logs. > > That's said, we should continue working together on it. > Send me your drafts and I'll help you to split them. The part I cannot do > by myself is about the explanations in commit logs. > Based on the principle "small commit", this is the rough idea. Btw, git tool will n't help here. Please have a quick read through. I will start the rework asap. Patch 1: copy examples/vhost/main.c/lib/librte_vhost/vhost/vhost_rxtx.c git mv examples/vhost/eventfd_link /lib/librte_vhost/ git mv examples/vhost/libvirt /lib/librte/vhost/libvirt git mv examples/vhost/vhost-net-cdev.c /lib/librte_vhost/ git mv examples/vhost/vhost-net-cdev.h /lib/librte_vhost/ git mv examples/vhost/virtio-net.* /lib/librte_vhost/ comment: As in previous patch set, vhost_rxtx.c is partly copied from main.c, here I decide to "copy" rather than "mv" main.c from example to vhost lib. The drawback is gitk couldn't recognize main.c and vhost_rxtx.c have the same index. The pros is even with mv, gitk couldn't recognize partly copy of vhost_rxtx.c, and later we could patch vhost example based on existing files. Will emphasize that vhost_rxtx.c is a purely copy without any modification in commit message. delete examples/vhost/Makefile as vhost example could no longer be compiled from here after until the example patch. Another option is leave all example files, and copy needed ones to vhost lib directory. The cons is gitk will treat all files in vhost lib as new files. patch 2: rename virtio-net.c to rte_virtio_net.h patch 3: remove zero copy logic in related files. patch 4: remove switching related logics in related files. patch 5: delete all functions in vhost_rxtx.c except four functions virtio_dev_(merge)_rx/tx and helper functions copy_mbuf_to_rings.. comment here: here virtio_dev_(merge)_rx/tx will refer non-existing functions like virtio_tx_route. I think it is ok, right? patch 6: remove virtio_dev_tx, and rename virtio_dev_merge_tx to virtio_dev_tx. patch virtio_dev_rx, virtio_dev_merge_rx and virtio_dev_tx will see if this can be further divided. patch 7: Other minior fixes, like change global vars to static vars patch 8: fixes plenty of serious coding style issues Patch 9: Vhost API patch Patch 10: Added identified TODO or FIXME patch 11: Add vhost lib makefiles. Patch 12...: Vhost example patch > Thanks > -- > Thomas
[dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf change
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Tuesday, September 30, 2014 3:44 AM > To: Xie, Huawei > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf > change > > > There is no rte_pktmbuf structure in mbuf now. Its fields are merged to > > rte_mbuf structure. > > > > Signed-off-by: Huawei Xie > > This patch shouldn't appear but should be merged with your previous work. > > -- > Thomas Hi Thomas: I would rework the patch according to your comment. I don't get clear about this comment. Do you mean that recreate the patch set based on the example that already has this mbuf change? Some of the background you might not know: I fully understand your concern here to make it a better patch and I fully agree with you total comments. This is really a special case. You know it is transform of thousand lines of code with modifications. Sometimes a simple change could take me more than one day to rework the patch, lines of lines manual check. I have already spent more than one week of time merely on the patch format itself. :(. Could we possibly treat it specially when we have comment whether the patch can be split/merged better?
[dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf change
> There is no rte_pktmbuf structure in mbuf now. Its fields are merged to > rte_mbuf structure. > > Signed-off-by: Huawei Xie This patch shouldn't appear but should be merged with your previous work. -- Thomas
[dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf change
There is no rte_pktmbuf structure in mbuf now. Its fields are merged to
rte_mbuf structure.
Signed-off-by: Huawei Xie
---
lib/librte_vhost/vhost_rxtx.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 81368e6..688e661 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -145,7 +145,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
struct rte_mbuf **pkts,
/* Copy mbuf data to buffer */
/* TODO fixme for sg mbuf and the case that desc couldn't hold
the mbuf data */
rte_memcpy((void *)(uintptr_t)buff_addr,
- (const void *)buff->pkt.data,
+ rte_pktmbuf_mtod(buff, const void *),
rte_pktmbuf_data_len(buff));
VHOST_PRINT_PACKET(dev, (uintptr_t)buff_addr,
rte_pktmbuf_data_len(buff), 0);
@@ -307,7 +307,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint16_t
res_base_idx,
* This current segment complete, need continue to
* check if the whole packet complete or not.
*/
- pkt = pkt->pkt.next;
+ pkt = pkt->next;
if (pkt != NULL) {
/*
* There are more segments.
@@ -411,7 +411,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t
queue_id, struct rte_mbuf *
uint32_t secure_len = 0;
uint16_t need_cnt;
uint32_t vec_idx = 0;
- uint32_t pkt_len = pkts[pkt_idx]->pkt.pkt_len + vq->vhost_hlen;
+ uint32_t pkt_len = pkts[pkt_idx]->pkt_len + vq->vhost_hlen;
uint16_t i, id;
do {
@@ -631,8 +631,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t
queue_id, struct rte_me
* while the virtio buffer in TX vring has
* more data to be copied.
*/
- cur->pkt.data_len = seg_offset;
- m->pkt.pkt_len += seg_offset;
+ cur->data_len = seg_offset;
+ m->pkt_len += seg_offset;
/* Allocate mbuf and populate the structure. */
cur = rte_pktmbuf_alloc(mbuf_pool);
if (unlikely(cur == NULL)) {
@@ -644,7 +644,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t
queue_id, struct rte_me
}
seg_num++;
- prev->pkt.next = cur;
+ prev->next = cur;
prev = cur;
seg_offset = 0;
seg_avail = cur->buf_len - RTE_PKTMBUF_HEADROOM;
@@ -660,8 +660,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t
queue_id, struct rte_me
* room to accomodate more
* data.
*/
- cur->pkt.data_len = seg_offset;
- m->pkt.pkt_len += seg_offset;
+ cur->data_len = seg_offset;
+ m->pkt_len += seg_offset;
/*
* Allocate an mbuf and
* populate the structure.
@@ -678,7 +678,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t
queue_id, struct rte_me
break;
}
seg_num++;
- prev->pkt.next = cur;
+ prev->next = cur;
prev = cur;
seg_offset = 0;
seg_avail = cur->buf_len -
RTE_PKTMBUF_HEADROOM;
@@ -697,8 +697,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t
queue_id, struct rte_me
desc->len, 0);
} else {
/* The whole packet completes. */
- cur->pkt.data_len = seg_offset;
- m->pkt.pkt_len += seg_offset;
+
