-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vdpa/virtio: Optimize device close time #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
priv->dev_work_flag = 0;
Please use macro instead of 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (priv->dev_work_flag == VIRTIO_VDPA_DEV_CLOSE_WORK_ERR) {
DRV_LOG(ERR, "%s is dev close work had err", vdev->device->name);
return -EINVAL;
}
Why not check VIRTIO_VDPA_DEV_CLOSE_WORK_DONE?
If it still work on it no done yeah.
5b94962
to
b6bd8e6
Compare
Move DMA unmap and state resoter work to work thread Signed-off-by: Kailiang Zhou <kailiangz@nvidia.com>
a2151db
to
614ee88
Compare
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
} | ||
static inline bool | ||
virtio_vdpa_find_mem_reg(struct rte_vhost_mem_region *key,struct rte_vhost_memory *mem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
white space before struct rte_vhost_memory
uint32_t i = 0; | ||
struct rte_vhost_mem_region *reg; | ||
|
||
if (mem == NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calller cannot pass null. please remove this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can pass null when no mem map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kailiangz1 , who can pass null? vhost from qemu side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 669, if previous no mem map, it will be NULL
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
static inline bool | ||
virtio_vdpa_find_mem_reg(struct rte_vhost_mem_region *key,struct rte_vhost_memory *mem) | ||
{ | ||
uint32_t i = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to zero initialize here, below for loop already doe it.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
} | ||
static inline bool | ||
virtio_vdpa_find_mem_reg(struct rte_vhost_mem_region *key,struct rte_vhost_memory *mem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make key and mem const pointer as this search routine is not going to modify anything.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
if (priv->mem) { | ||
for (i = 0; i < priv->mem->nregions; i++) { | ||
reg = &priv->mem->regions[i]; | ||
if (virtio_vdpa_find_mem_reg(reg, cur_mem)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parsing the code in the else to do the actual work is very confusing.
do it as,
ret = virtio_vdpa_find_mem_reg();
if (ret == 0) {
unmap code()
} else {
debug print
}
DRV_LOG(ERR, "%s DMA unmap failed ret:%d", | ||
priv->vdev->device->name, ret); | ||
/* Map reagion dind't exsit in privious */ | ||
for (i = 0; i < cur_mem->nregions; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and also have the error unwinding routine label to unmap or map the memory if the for loop fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not necessary to unwinding because mem map is to map the mem, if it exist, we will no map them. caller can call again to recover. Map exist mem no harm.
drivers/vdpa/virtio/virtio_vdpa.c
Outdated
if (ret < 0) { | ||
DRV_LOG(ERR, "%s DMA unmap failed ret:%d", | ||
priv->vdev->device->name, ret); | ||
/* Map reagion dind't exsit in privious */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment should be,
/* Map the region if it doesn't exist yet */
priv->vdev->device->name, ret); | ||
goto exit; | ||
/* Unmap reagion dind't exsit in current */ | ||
if (priv->mem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to have two different helper routines for map and unmap and call them here.
lib/vhost/vhost_user.c
Outdated
@@ -1445,6 +1445,9 @@ vhost_user_set_mem_table(struct virtio_net **pdev, | |||
if (dev->async_copy && rte_vfio_is_enabled("vfio")) | |||
async_dma_map(dev, true); | |||
|
|||
if ((dev->vdpa_dev) && (dev->vdpa_dev->ops->set_mem_table)) | |||
dev->vdpa_dev->ops->set_mem_table(dev->vdpa_dev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good work to have explicit call, instead of doing implicit in the dev_config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not understand. Can you show me the code example?
d3db83c
to
638afe8
Compare
Add new set mem operation, save dma map time when device config Signed-off-by: Kailiang Zhou <kailiangz@nvidia.com>
638afe8
to
b31739b
Compare
Move DMA unmap and state resoter work to work thread
Signed-off-by: Kailiang Zhou kailiangz@nvidia.com