[dpdk-dev,v5,05/11] lib/librte_vhost: merge Oliver's mbuf change
Commit Message
There is no rte_pktmbuf structure in mbuf now. Its fields are merged to
rte_mbuf structure.
Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
lib/librte_vhost/vhost_rxtx.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
Comments
> There is no rte_pktmbuf structure in mbuf now. Its fields are merged to
> rte_mbuf structure.
>
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
This patch shouldn't appear but should be merged with your previous work.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, September 30, 2014 3:44 AM
> To: Xie, Huawei
> Cc: dev@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 <huawei.xie@intel.com>
>
> 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?
Hi Huawei,
My response as below.
Thanks
Changchun
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xie, Huawei
> Sent: Tuesday, September 30, 2014 10:41 AM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's
> mbuf change
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Tuesday, September 30, 2014 3:44 AM
> > To: Xie, Huawei
> > Cc: dev@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 <huawei.xie@intel.com>
> >
> > 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?
My understanding is as follows:
Basically every patch file need base on the latest commit, so need merge all changes which have already been applied into mainline,
No only mbuf related change and mergeable feature but also other fix.
"git fetch" and "git pull origin master" may help you,
If there are conflicts, need resolve them.
And you don't need redo all works from scratch to regenerate the patch files,
Maybe based on your current commit and do the merge work and resolve some conflicts are the most rework task.
>
> 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?
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
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, September 30, 2014 12:46 PM
> To: Xie, Huawei
> Cc: dev@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
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.
@@ -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;
+ cur->data_len = seg_offset;
+ m->pkt_len += seg_offset;
vb_avail = 0;
}
}
@@ -709,7 +709,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id, struct rte_me
if (unlikely(alloc_err == 1))
break;
- m->pkt.nb_segs = seg_num;
+ m->nb_segs = seg_num;
pkts[entry_success] = m;
vq->last_used_idx++;