On 05/04/2017 09:51 PM, lixi...@cmss.chinamobile.com wrote: > From: Xiubo Li <lixi...@cmss.chinamobile.com> > > The fifo type waiter list will hold the udevs who are waiting for the > blocks from the data global pool. The unmap thread will try to feed the > first udevs in waiter list, if the global free blocks available are > not enough, it will stop traversing the list and abort waking up the > others. > > Signed-off-by: Xiubo Li <lixi...@cmss.chinamobile.com> > --- > drivers/target/target_core_user.c | 82 > +++++++++++++++++++++++++++++++-------- > 1 file changed, 66 insertions(+), 16 deletions(-) > > diff --git a/drivers/target/target_core_user.c > b/drivers/target/target_core_user.c > index 9045837..10c99e7 100644 > --- a/drivers/target/target_core_user.c > +++ b/drivers/target/target_core_user.c > @@ -97,6 +97,7 @@ struct tcmu_hba { > > struct tcmu_dev { > struct list_head node; > + struct list_head waiter; > > struct se_device se_dev; > > @@ -123,7 +124,7 @@ struct tcmu_dev { > wait_queue_head_t wait_cmdr; > struct mutex cmdr_lock; > > - bool waiting_global; > + uint32_t waiting_blocks; > uint32_t dbi_max; > uint32_t dbi_thresh; > DECLARE_BITMAP(data_bitmap, DATA_BLOCK_BITS); > @@ -165,6 +166,10 @@ struct tcmu_cmd { > static DEFINE_MUTEX(root_udev_mutex); > static LIST_HEAD(root_udev); > > +/* The data blocks global pool waiter list */ > +static DEFINE_MUTEX(root_udev_waiter_mutex);
Sorry for the delay. Could you just add a comment about the lock ordering. It will be helpful to new engineers/reviewers to check for errors. > +static LIST_HEAD(root_udev_waiter); > + > static atomic_t global_db_count = ATOMIC_INIT(0); > > static struct kmem_cache *tcmu_cmd_cache; > @@ -195,6 +200,11 @@ enum tcmu_multicast_groups { > #define tcmu_cmd_set_dbi(cmd, index) ((cmd)->dbi[(cmd)->dbi_cur++] = (index)) > #define tcmu_cmd_get_dbi(cmd) ((cmd)->dbi[(cmd)->dbi_cur++]) > > +static inline bool is_in_waiter_list(struct tcmu_dev *udev) > +{ > + return !!udev->waiting_blocks; > +} > + > static void tcmu_cmd_free_data(struct tcmu_cmd *tcmu_cmd, uint32_t len) > { > struct tcmu_dev *udev = tcmu_cmd->tcmu_dev; > @@ -250,8 +260,6 @@ static bool tcmu_get_empty_blocks(struct tcmu_dev *udev, > { > int i; > > - udev->waiting_global = false; > - > for (i = tcmu_cmd->dbi_cur; i < tcmu_cmd->dbi_cnt; i++) { > if (!tcmu_get_empty_block(udev, tcmu_cmd)) > goto err; > @@ -259,9 +267,7 @@ static bool tcmu_get_empty_blocks(struct tcmu_dev *udev, > return true; > > err: > - udev->waiting_global = true; > - /* Try to wake up the unmap thread */ > - wake_up(&unmap_wait); > + udev->waiting_blocks += tcmu_cmd->dbi_cnt - i; > return false; > } > > @@ -671,6 +677,7 @@ static inline size_t tcmu_cmd_get_cmd_size(struct > tcmu_cmd *tcmu_cmd, > > while (!is_ring_space_avail(udev, tcmu_cmd, command_size, data_length)) > { > int ret; > + bool is_waiting_blocks = !!udev->waiting_blocks; You can use your helper is_in_waiter_list(). > DEFINE_WAIT(__wait); > > prepare_to_wait(&udev->wait_cmdr, &__wait, TASK_INTERRUPTIBLE); > @@ -688,6 +695,18 @@ static inline size_t tcmu_cmd_get_cmd_size(struct > tcmu_cmd *tcmu_cmd, > return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > } > > + /* > + * Try to insert the current udev to waiter list and > + * then wake up the unmap thread > + */ > + if (is_waiting_blocks) { > + mutex_lock(&root_udev_waiter_mutex); > + list_add_tail(&udev->waiter, &root_udev_waiter); > + mutex_unlock(&root_udev_waiter_mutex); > + > + wake_up(&unmap_wait); > + } Is this supposed to go before the schedule_timeout call? If we are here and schedule_timeout returned non zero then our wait_cmdr has been woken up from the unmap thread because it freed some space, so we do not want to again add ourself and call unmap again. > + > mutex_lock(&udev->cmdr_lock); > > /* We dropped cmdr_lock, cmd_head is stale */ > @@ -902,8 +921,6 @@ static unsigned int tcmu_handle_completions(struct > tcmu_dev *udev) > if (mb->cmd_tail == mb->cmd_head) > del_timer(&udev->timeout); /* no more pending cmds */ > > - wake_up(&udev->wait_cmdr); > - > return handled; > } > > @@ -996,7 +1013,17 @@ static int tcmu_irqcontrol(struct uio_info *info, s32 > irq_on) > struct tcmu_dev *tcmu_dev = container_of(info, struct tcmu_dev, > uio_info); > > mutex_lock(&tcmu_dev->cmdr_lock); > - tcmu_handle_completions(tcmu_dev); > + /* > + * If the current udev is also in waiter list, this will > + * make sure that the other waiters in list be feeded ahead > + * of it. > + */ > + if (is_in_waiter_list(tcmu_dev)) { > + wake_up(&unmap_wait); > + } else { > + tcmu_handle_completions(tcmu_dev); > + wake_up(&tcmu_dev->wait_cmdr); > + } > mutex_unlock(&tcmu_dev->cmdr_lock); > > return 0; > @@ -1231,7 +1258,7 @@ static int tcmu_configure_device(struct se_device *dev) > udev->data_off = CMDR_SIZE; > udev->data_size = DATA_SIZE; > udev->dbi_thresh = 0; /* Default in Idle state */ > - udev->waiting_global = false; > + udev->waiting_blocks = 0; > > /* Initialise the mailbox of the ring buffer */ > mb = udev->mb_addr; > @@ -1345,6 +1372,13 @@ static void tcmu_free_device(struct se_device *dev) > list_del(&udev->node); > mutex_unlock(&root_udev_mutex); > > + mutex_lock(&root_udev_waiter_mutex); > + mutex_lock(&udev->cmdr_lock); > + if (is_in_waiter_list(udev)) > + list_del(&udev->waiter); > + mutex_unlock(&udev->cmdr_lock); > + mutex_unlock(&root_udev_waiter_mutex); > + > vfree(udev->mb_addr); > > /* Upper layer should drain all requests before calling this */ > @@ -1550,6 +1584,7 @@ static int unmap_thread_fn(void *data) > struct tcmu_dev *udev; > loff_t off; > uint32_t start, end, block; > + static uint32_t free_blocks; > struct page *page; > int i; > > @@ -1571,7 +1606,7 @@ static int unmap_thread_fn(void *data) > tcmu_handle_completions(udev); > > /* Skip the udevs waiting the global pool or in idle */ > - if (udev->waiting_global || !udev->dbi_thresh) { > + if (is_in_waiter_list(udev) || !udev->dbi_thresh) { > mutex_unlock(&udev->cmdr_lock); > continue; > } > @@ -1607,17 +1642,32 @@ static int unmap_thread_fn(void *data) > } > } > mutex_unlock(&udev->cmdr_lock); > + > + free_blocks += end - start; > } > + mutex_unlock(&root_udev_mutex); > > /* > * Try to wake up the udevs who are waiting > - * for the global data pool. > + * for the global data pool blocks. > */ > - list_for_each_entry(udev, &root_udev, node) { > - if (udev->waiting_global) > - wake_up(&udev->wait_cmdr); > + mutex_lock(&root_udev_waiter_mutex); > + list_for_each_entry(udev, &root_udev_waiter, waiter) { > + mutex_lock(&udev->cmdr_lock); > + if (udev->waiting_blocks < free_blocks) { > + mutex_unlock(&udev->cmdr_lock); > + break; > + } > + > + free_blocks -= udev->waiting_blocks; > + udev->waiting_blocks = 0; > + mutex_unlock(&udev->cmdr_lock); > + > + list_del(&udev->waiter); > + > + wake_up(&udev->wait_cmdr); > } > - mutex_unlock(&root_udev_mutex); > + mutex_unlock(&root_udev_waiter_mutex); > } > > return 0; >