| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZBH9ClO15oAf+kJB@localhost.localdomain>
Date: Wed, 15 Mar 2023 18:14:50 +0100
From: Michal Kubiak <michal.kubiak@...el.com>
To: Praveen Kaligineedi <pkaligineedi@...gle.com>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
<maciej.fijalkowski@...el.com>,
Jeroen de Borst <jeroendb@...gle.com>
Subject: Re: [PATCH net-next v3 2/5] gve: Changes to add new TX queues
On Mon, Mar 13, 2023 at 01:26:37PM -0700, Praveen Kaligineedi wrote:
> Changes to enable adding and removing TX queues without calling
> gve_close() and gve_open().
>
> Made the following changes:
> 1) priv->tx, priv->rx and priv->qpls arrays are allocated based on
> max tx queues and max rx queues
> 2) Changed gve_adminq_create_tx_queues(), gve_adminq_destroy_tx_queues(),
> gve_tx_alloc_rings() and gve_tx_free_rings() functions to add/remove a
> subset of TX queues rather than all the TX queues.
>
> Signed-off-by: Praveen Kaligineedi <pkaligineedi@...gle.com>
> Reviewed-by: Jeroen de Borst <jeroendb@...gle.com>
>
Reviewed-by: Michal Kubiak <michal.kubiak@...el.com>
> ---
> Changed in v2:
> - Added this patch to address the issue raised by Jakub Kicinski about
> implications of resource allocation failing after reconfig.
>
> Changed in v3:
> - No changes
> ---
> drivers/net/ethernet/google/gve/gve.h | 45 +++++++----
> drivers/net/ethernet/google/gve/gve_adminq.c | 8 +-
> drivers/net/ethernet/google/gve/gve_adminq.h | 4 +-
> drivers/net/ethernet/google/gve/gve_main.c | 83 ++++++++++++++------
> drivers/net/ethernet/google/gve/gve_rx.c | 2 +-
> drivers/net/ethernet/google/gve/gve_tx.c | 12 +--
> 6 files changed, 104 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index f52f23198278..f354a6448c25 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -798,16 +798,35 @@ static inline u32 gve_num_rx_qpls(struct gve_priv *priv)
> return priv->rx_cfg.num_queues;
> }
>
> +static inline u32 gve_tx_qpl_id(struct gve_priv *priv, int tx_qid)
> +{
> + return tx_qid;
> +}
> +
> +static inline u32 gve_rx_qpl_id(struct gve_priv *priv, int rx_qid)
> +{
> + return priv->tx_cfg.max_queues + rx_qid;
> +}
> +
> +static inline u32 gve_tx_start_qpl_id(struct gve_priv *priv)
> +{
> + return gve_tx_qpl_id(priv, 0);
> +}
> +
> +static inline u32 gve_rx_start_qpl_id(struct gve_priv *priv)
> +{
> + return gve_rx_qpl_id(priv, 0);
> +}
> +
> /* Returns a pointer to the next available tx qpl in the list of qpls
> */
> static inline
> -struct gve_queue_page_list *gve_assign_tx_qpl(struct gve_priv *priv)
> +struct gve_queue_page_list *gve_assign_tx_qpl(struct gve_priv *priv, int tx_qid)
> {
> - int id = find_first_zero_bit(priv->qpl_cfg.qpl_id_map,
> - priv->qpl_cfg.qpl_map_size);
> + int id = gve_tx_qpl_id(priv, tx_qid);
>
> - /* we are out of tx qpls */
> - if (id >= gve_num_tx_qpls(priv))
> + /* QPL already in use */
> + if (test_bit(id, priv->qpl_cfg.qpl_id_map))
> return NULL;
>
> set_bit(id, priv->qpl_cfg.qpl_id_map);
> @@ -817,14 +836,12 @@ struct gve_queue_page_list *gve_assign_tx_qpl(struct gve_priv *priv)
> /* Returns a pointer to the next available rx qpl in the list of qpls
> */
> static inline
> -struct gve_queue_page_list *gve_assign_rx_qpl(struct gve_priv *priv)
> +struct gve_queue_page_list *gve_assign_rx_qpl(struct gve_priv *priv, int rx_qid)
> {
> - int id = find_next_zero_bit(priv->qpl_cfg.qpl_id_map,
> - priv->qpl_cfg.qpl_map_size,
> - gve_num_tx_qpls(priv));
> + int id = gve_rx_qpl_id(priv, rx_qid);
>
> - /* we are out of rx qpls */
> - if (id == gve_num_tx_qpls(priv) + gve_num_rx_qpls(priv))
> + /* QPL already in use */
> + if (test_bit(id, priv->qpl_cfg.qpl_id_map))
> return NULL;
>
> set_bit(id, priv->qpl_cfg.qpl_id_map);
> @@ -843,7 +860,7 @@ static inline void gve_unassign_qpl(struct gve_priv *priv, int id)
> static inline enum dma_data_direction gve_qpl_dma_dir(struct gve_priv *priv,
> int id)
> {
> - if (id < gve_num_tx_qpls(priv))
> + if (id < gve_rx_start_qpl_id(priv))
> return DMA_TO_DEVICE;
> else
> return DMA_FROM_DEVICE;
> @@ -869,8 +886,8 @@ void gve_free_page(struct device *dev, struct page *page, dma_addr_t dma,
> /* tx handling */
> netdev_tx_t gve_tx(struct sk_buff *skb, struct net_device *dev);
> bool gve_tx_poll(struct gve_notify_block *block, int budget);
> -int gve_tx_alloc_rings(struct gve_priv *priv);
> -void gve_tx_free_rings_gqi(struct gve_priv *priv);
> +int gve_tx_alloc_rings(struct gve_priv *priv, int start_id, int num_rings);
> +void gve_tx_free_rings_gqi(struct gve_priv *priv, int start_id, int num_rings);
> u32 gve_tx_load_event_counter(struct gve_priv *priv,
> struct gve_tx_ring *tx);
> bool gve_tx_clean_pending(struct gve_priv *priv, struct gve_tx_ring *tx);
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 60061288ad9d..252974202a3f 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -516,12 +516,12 @@ static int gve_adminq_create_tx_queue(struct gve_priv *priv, u32 queue_index)
> return gve_adminq_issue_cmd(priv, &cmd);
> }
>
> -int gve_adminq_create_tx_queues(struct gve_priv *priv, u32 num_queues)
> +int gve_adminq_create_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues)
> {
> int err;
> int i;
>
> - for (i = 0; i < num_queues; i++) {
> + for (i = start_id; i < start_id + num_queues; i++) {
> err = gve_adminq_create_tx_queue(priv, i);
> if (err)
> return err;
> @@ -604,12 +604,12 @@ static int gve_adminq_destroy_tx_queue(struct gve_priv *priv, u32 queue_index)
> return 0;
> }
>
> -int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 num_queues)
> +int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues)
> {
> int err;
> int i;
>
> - for (i = 0; i < num_queues; i++) {
> + for (i = start_id; i < start_id + num_queues; i++) {
> err = gve_adminq_destroy_tx_queue(priv, i);
> if (err)
> return err;
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
> index cf29662e6ad1..f894beb3deaf 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.h
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.h
> @@ -410,8 +410,8 @@ int gve_adminq_configure_device_resources(struct gve_priv *priv,
> dma_addr_t db_array_bus_addr,
> u32 num_ntfy_blks);
> int gve_adminq_deconfigure_device_resources(struct gve_priv *priv);
> -int gve_adminq_create_tx_queues(struct gve_priv *priv, u32 num_queues);
> -int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 queue_id);
> +int gve_adminq_create_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues);
> +int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues);
> int gve_adminq_create_rx_queues(struct gve_priv *priv, u32 num_queues);
> int gve_adminq_destroy_rx_queues(struct gve_priv *priv, u32 queue_id);
> int gve_adminq_register_page_list(struct gve_priv *priv,
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 3cfdeeb74f60..160ca77c2751 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -584,11 +584,26 @@ static void gve_remove_napi(struct gve_priv *priv, int ntfy_idx)
>
> static int gve_register_qpls(struct gve_priv *priv)
> {
> - int num_qpls = gve_num_tx_qpls(priv) + gve_num_rx_qpls(priv);
> + int start_id;
> int err;
> int i;
>
> - for (i = 0; i < num_qpls; i++) {
> + start_id = gve_tx_start_qpl_id(priv);
> + for (i = start_id; i < start_id + gve_num_tx_qpls(priv); i++) {
> + err = gve_adminq_register_page_list(priv, &priv->qpls[i]);
> + if (err) {
> + netif_err(priv, drv, priv->dev,
> + "failed to register queue page list %d\n",
> + priv->qpls[i].id);
> + /* This failure will trigger a reset - no need to clean
> + * up
> + */
> + return err;
> + }
> + }
> +
> + start_id = gve_rx_start_qpl_id(priv);
> + for (i = start_id; i < start_id + gve_num_rx_qpls(priv); i++) {
> err = gve_adminq_register_page_list(priv, &priv->qpls[i]);
> if (err) {
> netif_err(priv, drv, priv->dev,
> @@ -605,11 +620,24 @@ static int gve_register_qpls(struct gve_priv *priv)
>
> static int gve_unregister_qpls(struct gve_priv *priv)
> {
> - int num_qpls = gve_num_tx_qpls(priv) + gve_num_rx_qpls(priv);
> + int start_id;
> int err;
> int i;
>
> - for (i = 0; i < num_qpls; i++) {
> + start_id = gve_tx_start_qpl_id(priv);
> + for (i = start_id; i < start_id + gve_num_tx_qpls(priv); i++) {
> + err = gve_adminq_unregister_page_list(priv, priv->qpls[i].id);
> + /* This failure will trigger a reset - no need to clean up */
> + if (err) {
> + netif_err(priv, drv, priv->dev,
> + "Failed to unregister queue page list %d\n",
> + priv->qpls[i].id);
> + return err;
> + }
> + }
> +
> + start_id = gve_rx_start_qpl_id(priv);
> + for (i = start_id; i < start_id + gve_num_rx_qpls(priv); i++) {
> err = gve_adminq_unregister_page_list(priv, priv->qpls[i].id);
> /* This failure will trigger a reset - no need to clean up */
> if (err) {
> @@ -628,7 +656,7 @@ static int gve_create_rings(struct gve_priv *priv)
> int err;
> int i;
>
> - err = gve_adminq_create_tx_queues(priv, num_tx_queues);
> + err = gve_adminq_create_tx_queues(priv, 0, num_tx_queues);
> if (err) {
> netif_err(priv, drv, priv->dev, "failed to create %d tx queues\n",
> num_tx_queues);
> @@ -695,10 +723,10 @@ static void add_napi_init_sync_stats(struct gve_priv *priv,
> }
> }
>
> -static void gve_tx_free_rings(struct gve_priv *priv)
> +static void gve_tx_free_rings(struct gve_priv *priv, int start_id, int num_rings)
> {
> if (gve_is_gqi(priv)) {
> - gve_tx_free_rings_gqi(priv);
> + gve_tx_free_rings_gqi(priv, start_id, num_rings);
> } else {
> gve_tx_free_rings_dqo(priv);
> }
> @@ -709,20 +737,20 @@ static int gve_alloc_rings(struct gve_priv *priv)
> int err;
>
> /* Setup tx rings */
> - priv->tx = kvcalloc(priv->tx_cfg.num_queues, sizeof(*priv->tx),
> + priv->tx = kvcalloc(priv->tx_cfg.max_queues, sizeof(*priv->tx),
> GFP_KERNEL);
> if (!priv->tx)
> return -ENOMEM;
>
> if (gve_is_gqi(priv))
> - err = gve_tx_alloc_rings(priv);
> + err = gve_tx_alloc_rings(priv, 0, gve_num_tx_queues(priv));
> else
> err = gve_tx_alloc_rings_dqo(priv);
> if (err)
> goto free_tx;
>
> /* Setup rx rings */
> - priv->rx = kvcalloc(priv->rx_cfg.num_queues, sizeof(*priv->rx),
> + priv->rx = kvcalloc(priv->rx_cfg.max_queues, sizeof(*priv->rx),
> GFP_KERNEL);
> if (!priv->rx) {
> err = -ENOMEM;
> @@ -747,7 +775,7 @@ static int gve_alloc_rings(struct gve_priv *priv)
> kvfree(priv->rx);
> priv->rx = NULL;
> free_tx_queue:
> - gve_tx_free_rings(priv);
> + gve_tx_free_rings(priv, 0, gve_num_tx_queues(priv));
> free_tx:
> kvfree(priv->tx);
> priv->tx = NULL;
> @@ -759,7 +787,7 @@ static int gve_destroy_rings(struct gve_priv *priv)
> int num_tx_queues = gve_num_tx_queues(priv);
> int err;
>
> - err = gve_adminq_destroy_tx_queues(priv, num_tx_queues);
> + err = gve_adminq_destroy_tx_queues(priv, 0, num_tx_queues);
> if (err) {
> netif_err(priv, drv, priv->dev,
> "failed to destroy tx queues\n");
> @@ -797,7 +825,7 @@ static void gve_free_rings(struct gve_priv *priv)
> ntfy_idx = gve_tx_idx_to_ntfy(priv, i);
> gve_remove_napi(priv, ntfy_idx);
> }
> - gve_tx_free_rings(priv);
> + gve_tx_free_rings(priv, 0, num_tx_queues);
> kvfree(priv->tx);
> priv->tx = NULL;
> }
> @@ -894,40 +922,46 @@ static void gve_free_queue_page_list(struct gve_priv *priv, u32 id)
> qpl->page_buses[i], gve_qpl_dma_dir(priv, id));
>
> kvfree(qpl->page_buses);
> + qpl->page_buses = NULL;
> free_pages:
> kvfree(qpl->pages);
> + qpl->pages = NULL;
> priv->num_registered_pages -= qpl->num_entries;
> }
>
> static int gve_alloc_qpls(struct gve_priv *priv)
> {
> - int num_qpls = gve_num_tx_qpls(priv) + gve_num_rx_qpls(priv);
> + int max_queues = priv->tx_cfg.max_queues + priv->rx_cfg.max_queues;
> + int start_id;
> int i, j;
> int err;
>
> - if (num_qpls == 0)
> + if (priv->queue_format != GVE_GQI_QPL_FORMAT)
> return 0;
>
> - priv->qpls = kvcalloc(num_qpls, sizeof(*priv->qpls), GFP_KERNEL);
> + priv->qpls = kvcalloc(max_queues, sizeof(*priv->qpls), GFP_KERNEL);
> if (!priv->qpls)
> return -ENOMEM;
>
> - for (i = 0; i < gve_num_tx_qpls(priv); i++) {
> + start_id = gve_tx_start_qpl_id(priv);
> + for (i = start_id; i < start_id + gve_num_tx_qpls(priv); i++) {
> err = gve_alloc_queue_page_list(priv, i,
> priv->tx_pages_per_qpl);
> if (err)
> goto free_qpls;
> }
> - for (; i < num_qpls; i++) {
> +
> + start_id = gve_rx_start_qpl_id(priv);
> + for (i = start_id; i < start_id + gve_num_rx_qpls(priv); i++) {
> err = gve_alloc_queue_page_list(priv, i,
> priv->rx_data_slot_cnt);
> if (err)
> goto free_qpls;
> }
>
> - priv->qpl_cfg.qpl_map_size = BITS_TO_LONGS(num_qpls) *
> + priv->qpl_cfg.qpl_map_size = BITS_TO_LONGS(max_queues) *
> sizeof(unsigned long) * BITS_PER_BYTE;
> - priv->qpl_cfg.qpl_id_map = kvcalloc(BITS_TO_LONGS(num_qpls),
> + priv->qpl_cfg.qpl_id_map = kvcalloc(BITS_TO_LONGS(max_queues),
> sizeof(unsigned long), GFP_KERNEL);
> if (!priv->qpl_cfg.qpl_id_map) {
> err = -ENOMEM;
> @@ -940,23 +974,26 @@ static int gve_alloc_qpls(struct gve_priv *priv)
> for (j = 0; j <= i; j++)
> gve_free_queue_page_list(priv, j);
> kvfree(priv->qpls);
> + priv->qpls = NULL;
> return err;
> }
>
> static void gve_free_qpls(struct gve_priv *priv)
> {
> - int num_qpls = gve_num_tx_qpls(priv) + gve_num_rx_qpls(priv);
> + int max_queues = priv->tx_cfg.max_queues + priv->rx_cfg.max_queues;
> int i;
>
> - if (num_qpls == 0)
> + if (!priv->qpls)
> return;
>
> kvfree(priv->qpl_cfg.qpl_id_map);
> + priv->qpl_cfg.qpl_id_map = NULL;
>
> - for (i = 0; i < num_qpls; i++)
> + for (i = 0; i < max_queues; i++)
> gve_free_queue_page_list(priv, i);
>
> kvfree(priv->qpls);
> + priv->qpls = NULL;
> }
>
> /* Use this to schedule a reset when the device is capable of continuing
> diff --git a/drivers/net/ethernet/google/gve/gve_rx.c b/drivers/net/ethernet/google/gve/gve_rx.c
> index db1c74b1d7d3..051a15e4f1af 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx.c
> @@ -124,7 +124,7 @@ static int gve_prefill_rx_pages(struct gve_rx_ring *rx)
> return -ENOMEM;
>
> if (!rx->data.raw_addressing) {
> - rx->data.qpl = gve_assign_rx_qpl(priv);
> + rx->data.qpl = gve_assign_rx_qpl(priv, rx->q_num);
> if (!rx->data.qpl) {
> kvfree(rx->data.page_info);
> rx->data.page_info = NULL;
> diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c
> index 0fb052ce9e0b..e24e73e74e33 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx.c
> @@ -195,7 +195,7 @@ static int gve_tx_alloc_ring(struct gve_priv *priv, int idx)
> tx->raw_addressing = priv->queue_format == GVE_GQI_RDA_FORMAT;
> tx->dev = &priv->pdev->dev;
> if (!tx->raw_addressing) {
> - tx->tx_fifo.qpl = gve_assign_tx_qpl(priv);
> + tx->tx_fifo.qpl = gve_assign_tx_qpl(priv, idx);
> if (!tx->tx_fifo.qpl)
> goto abort_with_desc;
> /* map Tx FIFO */
> @@ -233,12 +233,12 @@ static int gve_tx_alloc_ring(struct gve_priv *priv, int idx)
> return -ENOMEM;
> }
>
> -int gve_tx_alloc_rings(struct gve_priv *priv)
> +int gve_tx_alloc_rings(struct gve_priv *priv, int start_id, int num_rings)
> {
> int err = 0;
> int i;
>
> - for (i = 0; i < priv->tx_cfg.num_queues; i++) {
> + for (i = start_id; i < start_id + num_rings; i++) {
> err = gve_tx_alloc_ring(priv, i);
> if (err) {
> netif_err(priv, drv, priv->dev,
> @@ -251,17 +251,17 @@ int gve_tx_alloc_rings(struct gve_priv *priv)
> if (err) {
> int j;
>
> - for (j = 0; j < i; j++)
> + for (j = start_id; j < i; j++)
> gve_tx_free_ring(priv, j);
> }
> return err;
> }
>
> -void gve_tx_free_rings_gqi(struct gve_priv *priv)
> +void gve_tx_free_rings_gqi(struct gve_priv *priv, int start_id, int num_rings)
> {
> int i;
>
> - for (i = 0; i < priv->tx_cfg.num_queues; i++)
> + for (i = start_id; i < start_id + num_rings; i++)
> gve_tx_free_ring(priv, i);
> }
>
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>
Powered by blists - more mailing lists