Re: [PATCH v3] net/cnxk: avoid command copy from Tx queue

[Jerin Jacob <jerinjacobk@gmail.com>]

On Thu, Feb 10, 2022 at 3:43 PM <pbhagavatula@marvell.com> wrote:
>
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Tx command is prepared based on offloads enabled and stored in
> Tx queue structure at tx_queue_setup phase.
> In fastpath the command is copied from Tx queue to LMT line for
> all the packets.
> Since, the command contents are mostly constants we can move the
> command preparation to fastpath and avoid accessing Tx queue
> memory.
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>  v3 Changes:
>  - Rebase.
>  - Split patches.
>  - Refactoring large function.
>
>  v2 Changes:
>  - Rebase.
>  - Fix incorrect use of RoC API
>
>  drivers/common/cnxk/roc_io.h             |  33 ++++-
> +static void
> +cnxk_sso_tx_queue_data_init(struct cnxk_sso_evdev *dev, uint64_t *txq_data,
> +                           uint16_t eth_port_id, uint16_t tx_queue_id)
> +{
> +       uint64_t offset = 0;
> +       int i, j;
> +
> +       dev->max_queue_id[0] = RTE_MAX(dev->max_queue_id[0], eth_port_id);
> +       for (i = 1; i < eth_port_id; i++) {
> +               offset += (dev->max_queue_id[i - 1] + 1);
> +               txq_data[i] |= offset << 48;
> +       }
> +       dev->max_port_id = RTE_MAX(dev->max_port_id, eth_port_id);
> +       dev->max_queue_id[eth_port_id] =
> +               RTE_MAX(dev->max_queue_id[eth_port_id], tx_queue_id);
> +}
> +
> +static void
> +cnxk_sso_tx_queue_data_rewrite(struct cnxk_sso_evdev *dev, uint64_t *txq_data,
> +                              uint16_t eth_port_id, uint16_t tx_queue_id,
> +                              uint64_t *otxq_data, uint16_t max_port_id,
> +                              uint16_t max_queue_id)
> +{
> +       uint64_t offset = 0;
> +       int i, j;
> +
> +       for (i = 0; i < dev->max_queue_id[0] + 1; i++)
> +               txq_data[i] |= (otxq_data[i] & ~((BIT_ULL(16) - 1) << 48));
> +
> +       if (eth_port_id > max_port_id) {
> +               dev->max_queue_id[0] =
> +                       RTE_MAX(dev->max_queue_id[0], eth_port_id);
> +               dev->max_port_id = RTE_MAX(dev->max_port_id, eth_port_id);
> +
> +               for (i = 1; i < eth_port_id; i++) {
> +                       offset += (dev->max_queue_id[i - 1] + 1);
> +                       txq_data[i] |= offset << 48;
> +                       for (j = 0; (i < dev->max_port_id) &&
> +                                   (j < dev->max_queue_id[i] + 1);
> +                            j++)
> +                               txq_data[offset + j] =
> +                                       otxq_data[(otxq_data[i] >> 48) + j];
> +               }
> +               dev->max_queue_id[eth_port_id] =
> +                       RTE_MAX(dev->max_queue_id[eth_port_id], tx_queue_id);

Could you move this as a separate static function? Too much depth


> +       } else if (tx_queue_id > max_queue_id) {
> +               dev->max_queue_id[eth_port_id] =
> +                       RTE_MAX(dev->max_queue_id[eth_port_id], tx_queue_id);
> +               dev->max_port_id = RTE_MAX(max_port_id, eth_port_id);
> +               for (i = 1; i < max_port_id + 1; i++) {
> +                       offset += (dev->max_queue_id[i - 1] + 1);
> +                       txq_data[i] |= offset << 48;
> +                       for (j = 0; j < dev->max_queue_id[i] + 1; j++) {
> +                               if (i == eth_port_id && j > max_queue_id)
> +                                       continue;
> +                               txq_data[offset + j] =
> +                                       otxq_data[(otxq_data[i] >> 48) + j];
> +                       }
> +               }
> +       }
> +}

Could you move this as a separate static function? Too much depth


> +
>  static int
>  cnxk_sso_updt_tx_queue_data(const struct rte_eventdev *event_dev,
>                             uint16_t eth_port_id, uint16_t tx_queue_id,
>                             void *txq)
>  {
>         struct cnxk_sso_evdev *dev = cnxk_sso_pmd_priv(event_dev);
> +       uint16_t max_queue_id = dev->max_queue_id[eth_port_id];
>         uint16_t max_port_id = dev->max_port_id;
> -       uint64_t *txq_data = dev->tx_adptr_data;
> -
> -       if (txq_data == NULL || eth_port_id > max_port_id) {
> -               max_port_id = RTE_MAX(max_port_id, eth_port_id);
> -               txq_data = rte_realloc_socket(
> -                       txq_data,
> -                       (sizeof(uint64_t) * (max_port_id + 1) *
> -                        RTE_MAX_QUEUES_PER_PORT),
> -                       RTE_CACHE_LINE_SIZE, event_dev->data->socket_id);
> +       uint64_t offset = 0, row = 0;
> +       uint64_t *txq_data = NULL;
> +       size_t size = 0;
> +       int i, j;
> +
> +       if (((uint64_t)txq) & 0xFFFF000000000000)
> +               return -EINVAL;
> +
> +       if (dev->tx_adptr_data == NULL) {
> +               size = (eth_port_id + 1);
> +               size += (eth_port_id + tx_queue_id);
> +               row = 2 * eth_port_id;
> +       } else {
> +               if (eth_port_id > max_port_id) {
> +                       size = (RTE_MAX(eth_port_id, dev->max_queue_id[0]) + 1);
> +                       for (i = 1; i < eth_port_id; i++)
> +                               size += (dev->max_queue_id[i] + 1);
> +                       row = size;
> +                       size += (tx_queue_id + 1);
> +               } else if (tx_queue_id > max_queue_id) {
> +                       size = !eth_port_id ? tx_queue_id + 1 :
> +                                                   RTE_MAX(max_port_id,
> +                                                     dev->max_queue_id[0]) +
> +                                                     1;

See below
> +                       for (i = 1; i < max_port_id + 1; i++) {
> +                               if (i == eth_port_id) {
> +                                       row = size;
> +                                       size += tx_queue_id + 1;
> +                               } else {
> +                                       size += dev->max_queue_id[i] + 1;
> +                               }
> +                       }
> +               }
> +       }
Could you move this as a separate static function? Too much depth

The rest looks good.