Re: [PATCH v4 09/14] migration/multifd: Prepare to introduce DSA acceleration on the multifd path.

[Peter Xu <peterx@redhat.com>]

On Thu, Apr 25, 2024 at 02:21:12AM +0000, Hao Xiang wrote:
> 1. Refactor multifd_send_thread function.
> 2. Introduce the batch task structure in MultiFDSendParams.
> 
> Signed-off-by: Hao Xiang <hao.xiang@linux.dev>
> ---
>  include/qemu/dsa.h  | 51 +++++++++++++++++++++++++++++++++++++++++++--
>  migration/multifd.c |  5 +++++
>  migration/multifd.h |  2 ++
>  util/dsa.c          | 51 ++++++++++++++++++++++++++++++++++++++-------
>  4 files changed, 99 insertions(+), 10 deletions(-)
> 
> diff --git a/include/qemu/dsa.h b/include/qemu/dsa.h
> index e002652879..0c36e93016 100644
> --- a/include/qemu/dsa.h
> +++ b/include/qemu/dsa.h
> @@ -2,6 +2,7 @@
>  #define QEMU_DSA_H
>  
>  #include "qemu/error-report.h"
> +#include "exec/cpu-common.h"
>  #include "qemu/thread.h"
>  #include "qemu/queue.h"
>  
> @@ -42,6 +43,21 @@ typedef struct dsa_batch_task {
>      QSIMPLEQ_ENTRY(dsa_batch_task) entry;
>  } dsa_batch_task;
>  
> +#endif
> +
> +struct batch_task {
> +#ifdef CONFIG_DSA_OPT
> +    /* Address of each pages in pages */
> +    ram_addr_t *addr;
> +    /* Zero page checking results */
> +    bool *results;
> +    /* Batch task DSA specific implementation */
> +    struct dsa_batch_task *dsa_batch;
> +#endif
> +};
> +
> +#ifdef CONFIG_DSA_OPT
> +
>  /**
>   * @brief Initializes DSA devices.
>   *
> @@ -74,7 +90,7 @@ void dsa_cleanup(void);
>  bool dsa_is_running(void);
>  
>  /**
> - * @brief Initializes a buffer zero batch task.
> + * @brief Initializes a buffer zero DSA batch task.
>   *
>   * @param task A pointer to the batch task to initialize.
>   * @param results A pointer to an array of zero page checking results.
> @@ -102,9 +118,26 @@ void buffer_zero_batch_task_destroy(struct dsa_batch_task *task);
>   * @return Zero if successful, otherwise non-zero.
>   */
>  int
> -buffer_is_zero_dsa_batch_async(struct dsa_batch_task *batch_task,
> +buffer_is_zero_dsa_batch_async(struct batch_task *batch_task,
>                                 const void **buf, size_t count, size_t len);
>  
> +/**
> + * @brief Initializes a general buffer zero batch task.
> + *
> + * @param batch_size The number of zero page checking tasks in the batch.
> + * @return A pointer to the general batch task initialized.
> + */
> +struct batch_task *
> +batch_task_init(int batch_size);
> +
> +/**
> + * @brief Destroys a general buffer zero batch task.
> + *
> + * @param task A pointer to the general batch task to destroy.
> + */
> +void
> +batch_task_destroy(struct batch_task *task);
> +
>  #else
>  
>  static inline bool dsa_is_running(void)
> @@ -128,6 +161,20 @@ static inline void dsa_stop(void) {}
>  
>  static inline void dsa_cleanup(void) {}
>  
> +static inline int
> +buffer_is_zero_dsa_batch_async(struct batch_task *batch_task,
> +                               const void **buf, size_t count, size_t len)
> +{
> +    exit(1);
> +}
> +
> +static inline struct batch_task *batch_task_init(int batch_size)
> +{
> +    return NULL;
> +}
> +
> +static inline void batch_task_destroy(struct batch_task *task) {}

I feel like there're too many things exported for DSA.

For example, at least buffer_is_zero_dsa_batch_async() looks like not
needed to be exported, maybe what should be exported is
zero_page_detect_dsa()?

We also should avoid accessing dsa internal fields in multifd*.c generic
code, for example, I think we should avoid things like below:

MultiFDSendParams:
    struct batch_task *batch_task;

multifd_send_setup:

    if (dsa_init(dsa_parameter)) {
        error_setg(&local_err, "multifd: Sender failed to initialize DSA.");
        error_report_err(local_err);
        return false;
    }

    dsa_start();

    ...

    for (each_thread)
        p->batch_task = batch_task_init(page_count);

This is way too ugly...

We should have one multifd_dsa_send_setup() and call it once and for all,
internally you can do whatever you want, rewalk the thread pool and init
states.

The name "batch_task" isn't clear either on being consumed by DSA.  I'd
think something like "dsa_state" better.

So instead of above like:

struct batch_task {
#ifdef CONFIG_DSA_OPT
    /* Address of each pages in pages */
    ram_addr_t *addr;
    /* Zero page checking results */
    bool *results;
    /* Batch task DSA specific implementation */
    struct dsa_batch_task *dsa_batch;
#endif
};

The fields should always be defined (say, dsa_state), then:

struct dsa_state {
    /* Address of each pages in pages */
    ram_addr_t *addr;
    /* Zero page checking results */
    bool *results;
    /* Batch task DSA specific implementation */
    struct dsa_batch_task *dsa_batch;
};

MultiFDSendParams:
    ...
#ifdef CONFIG_DSA_OPT
    struct dsa_state *dsa_state;
#endif

> +
>  #endif
>  
>  #endif
> diff --git a/migration/multifd.c b/migration/multifd.c
> index f317bff077..cfd3a92f6c 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -13,6 +13,8 @@
>  #include "qemu/osdep.h"
>  #include "qemu/cutils.h"
>  #include "qemu/rcu.h"
> +#include "qemu/dsa.h"
> +#include "qemu/memalign.h"
>  #include "exec/target_page.h"
>  #include "sysemu/sysemu.h"
>  #include "exec/ramblock.h"
> @@ -780,6 +782,8 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>      p->name = NULL;
>      multifd_pages_clear(p->pages);
>      p->pages = NULL;
> +    batch_task_destroy(p->batch_task);
> +    p->batch_task = NULL;

Again, please try to export as less DSA relevant functions as possible.
Here IMHO we only need one dsa_state_destroy() on multifd_send_state, do
whatever inside.

>      p->packet_len = 0;
>      g_free(p->packet);
>      p->packet = NULL;
> @@ -1172,6 +1176,7 @@ bool multifd_send_setup(void)
>          qemu_sem_init(&p->sem_sync, 0);
>          p->id = i;
>          p->pages = multifd_pages_init(page_count);
> +        p->batch_task = batch_task_init(page_count);
>  
>          if (use_packets) {
>              p->packet_len = sizeof(MultiFDPacket_t)
> diff --git a/migration/multifd.h b/migration/multifd.h
> index c9d9b09239..16e27db5e9 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -135,6 +135,8 @@ typedef struct {
>       * pending_job != 0 -> multifd_channel can use it.
>       */
>      MultiFDPages_t *pages;
> +    /* Zero page checking batch task */
> +    struct batch_task *batch_task;
>  
>      /* thread local variables. No locking required */
>  
> diff --git a/util/dsa.c b/util/dsa.c
> index 5a2bf33651..4f695e58af 100644
> --- a/util/dsa.c
> +++ b/util/dsa.c
> @@ -802,7 +802,7 @@ buffer_zero_task_init_int(struct dsa_hw_desc *descriptor,
>  }
>  
>  /**
> - * @brief Initializes a buffer zero batch task.
> + * @brief Initializes a buffer zero DSA batch task.
>   *
>   * @param task A pointer to the batch task to initialize.
>   * @param results A pointer to an array of zero page checking results.
> @@ -1107,29 +1107,64 @@ void dsa_cleanup(void)
>   * @return Zero if successful, otherwise non-zero.
>   */
>  int
> -buffer_is_zero_dsa_batch_async(struct dsa_batch_task *batch_task,
> +buffer_is_zero_dsa_batch_async(struct batch_task *batch_task,
>                                 const void **buf, size_t count, size_t len)
>  {
> -    if (count <= 0 || count > batch_task->batch_size) {
> +    struct dsa_batch_task *dsa_batch = batch_task->dsa_batch;
> +
> +    if (count <= 0 || count > dsa_batch->batch_size) {
>          return -1;
>      }
>  
> -    assert(batch_task != NULL);
> +    assert(dsa_batch != NULL);
>      assert(len != 0);
>      assert(buf != NULL);
>  
>      if (count == 1) {
>          /* DSA doesn't take batch operation with only 1 task. */
> -        buffer_zero_dsa_async(batch_task, buf[0], len);
> +        buffer_zero_dsa_async(dsa_batch, buf[0], len);
>      } else {
> -        buffer_zero_dsa_batch_async(batch_task, buf, count, len);
> +        buffer_zero_dsa_batch_async(dsa_batch, buf, count, len);
>      }
>  
> -    buffer_zero_dsa_wait(batch_task);
> -    buffer_zero_cpu_fallback(batch_task);
> +    buffer_zero_dsa_wait(dsa_batch);
> +    buffer_zero_cpu_fallback(dsa_batch);
>  
>      return 0;
>  }
>  
> +/**
> + * @brief Initializes a general buffer zero batch task.
> + *
> + * @param batch_size The number of zero page checking tasks in the batch.
> + * @return A pointer to the general batch task initialized.
> + */
> +struct batch_task *
> +batch_task_init(int batch_size)
> +{
> +    struct batch_task *task = g_malloc0(sizeof(struct batch_task));
> +    task->addr = g_new0(ram_addr_t, batch_size);
> +    task->results = g_new0(bool, batch_size);
> +    task->dsa_batch = qemu_memalign(64, sizeof(struct dsa_batch_task));
> +    buffer_zero_batch_task_init(task->dsa_batch, task->results, batch_size);
> +
> +    return task;
> +}
> +
> +/**
> + * @brief Destroys a general buffer zero batch task.
> + *
> + * @param task A pointer to the general batch task to destroy.
> + */
> +void
> +batch_task_destroy(struct batch_task *task)
> +{
> +    g_free(task->addr);
> +    g_free(task->results);
> +    buffer_zero_batch_task_destroy(task->dsa_batch);
> +    qemu_vfree(task->dsa_batch);
> +    g_free(task);
> +}
> +
>  #endif
>  
> -- 
> 2.30.2
> 
> 

-- 
Peter Xu