mbox series

[net-next,v2,0/9] Add GVE Features

Message ID 20200901215149.2685117-1-awogbemila@google.com
Headers show
Series Add GVE Features | expand

Message

David Awogbemila Sept. 1, 2020, 9:51 p.m. UTC 
Changes from v1:
Note: Patches 9 to 18 in v1 are dropped and will be uploaded in another
patchset.
Patch 1: Use EOPPNOTSUPP instead of EINVAL for get_tunable and set_tunable
				 unhandled cases.
Patch 3: Do NOT register netdev earlier.
				 Retitle from "gve: Register netdev earlier" to
				 "gve: Use dev_info/err instead of netif_info/err."
Patch 5: Break up into guest->NIC stats, NIC->guest stats.
Patch 7 (patch 6 in v1): Use reverse christmas tree in declaring tail in
				 gve_adminq_issue_cmd.
Patch 8 (patch 7 in v1): Add link_status Up to make logging more uniform.
Patch 9 (patch 8 in v1): Correct declaration of link_speed_region to
				 __be64* to fix sparse warning.

Catherine Sullivan (2):
  gve: Use dev_info/err instead of netif_info/err.
  gve: Add support for dma_mask register

David Awogbemila (2):
  gve: NIC stats for report-stats and for ethtool
  gve: Enable Link Speed Reporting in the driver.

Kuo Zhao (3):
  gve: Get and set Rx copybreak via ethtool
  gve: Add stats for gve.
  gve: Add Gvnic stats AQ command and ethtool show/set-priv-flags.

Patricio Noyola (1):
  gve: Use link status register to report link status

Sagi Shahar (1):
  gve: Batch AQ commands for creating and destroying queues.

 drivers/net/ethernet/google/gve/gve.h         | 111 +++++-
 drivers/net/ethernet/google/gve/gve_adminq.c  | 316 +++++++++++++--
 drivers/net/ethernet/google/gve/gve_adminq.h  |  62 ++-
 drivers/net/ethernet/google/gve/gve_ethtool.c | 366 ++++++++++++++++--
 drivers/net/ethernet/google/gve/gve_main.c    | 336 ++++++++++++----
 .../net/ethernet/google/gve/gve_register.h    |   4 +-
 drivers/net/ethernet/google/gve/gve_rx.c      |  37 +-
 7 files changed, 1053 insertions(+), 179 deletions(-)

Comments

Andrew Lunn Sept. 1, 2020, 10:08 p.m. UTC | #1 
> @@ -1133,7 +1133,9 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto abort_with_db_bar;
>  	}
>  	SET_NETDEV_DEV(dev, &pdev->dev);
> +
>  	pci_set_drvdata(pdev, dev);
> +
>  	dev->ethtool_ops = &gve_ethtool_ops;
>  	dev->netdev_ops = &gve_netdev_ops;
>  	/* advertise features */
> @@ -1160,6 +1162,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	priv->state_flags = 0x0;
>  
>  	gve_set_probe_in_progress(priv);
> +
>  	priv->gve_wq = alloc_ordered_workqueue("gve", 0);
>  	if (!priv->gve_wq) {
>  		dev_err(&pdev->dev, "Could not allocate workqueue");
> @@ -1181,6 +1184,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	dev_info(&pdev->dev, "GVE version %s\n", gve_version_str);
>  	gve_clear_probe_in_progress(priv);
>  	queue_work(priv->gve_wq, &priv->service_task);
> +
>  	return 0;
>  
>  abort_with_wq:

No white space changes please. If you want these, put them into a
patch of there own.

      Andrew
Jakub Kicinski Sept. 2, 2020, 12:45 a.m. UTC | #2 
On Tue,  1 Sep 2020 14:51:45 -0700 David Awogbemila wrote:

> @@ -297,6 +317,22 @@ static inline void gve_clear_probe_in_progress(struct gve_priv *priv)
>  	clear_bit(GVE_PRIV_FLAGS_PROBE_IN_PROGRESS, &priv->service_task_flags);
>  }
>  
> +static inline bool gve_get_do_report_stats(struct gve_priv *priv)
> +{
> +	return test_bit(GVE_PRIV_FLAGS_DO_REPORT_STATS,
> +			&priv->service_task_flags);
> +}
> +
> +static inline void gve_set_do_report_stats(struct gve_priv *priv)
> +{
> +	set_bit(GVE_PRIV_FLAGS_DO_REPORT_STATS, &priv->service_task_flags);
> +}
> +
> +static inline void gve_clear_do_report_stats(struct gve_priv *priv)
> +{
> +	clear_bit(GVE_PRIV_FLAGS_DO_REPORT_STATS, &priv->service_task_flags);
> +}
> +
>  static inline bool gve_get_admin_queue_ok(struct gve_priv *priv)
>  {
>  	return test_bit(GVE_PRIV_FLAGS_ADMIN_QUEUE_OK, &priv->state_flags);
> @@ -357,6 +393,21 @@ static inline void gve_clear_napi_enabled(struct gve_priv *priv)
>  	clear_bit(GVE_PRIV_FLAGS_NAPI_ENABLED, &priv->state_flags);
>  }
>  
> +static inline bool gve_get_report_stats(struct gve_priv *priv)
> +{
> +	return test_bit(GVE_PRIV_FLAGS_REPORT_STATS, &priv->ethtool_flags);
> +}
> +
> +static inline void gve_set_report_stats(struct gve_priv *priv)

Please remove the unused helpers.

> +{
> +	set_bit(GVE_PRIV_FLAGS_REPORT_STATS, &priv->ethtool_flags);
> +}
> +
> +static inline void gve_clear_report_stats(struct gve_priv *priv)
> +{
> +	clear_bit(GVE_PRIV_FLAGS_REPORT_STATS, &priv->ethtool_flags);
> +}

> @@ -353,6 +377,54 @@ static int gve_set_tunable(struct net_device *netdev,
>  	}
>  }
>  
> +static u32 gve_get_priv_flags(struct net_device *netdev)
> +{
> +	struct gve_priv *priv = netdev_priv(netdev);
> +	u32 i, ret_flags = 0;
> +
> +	for (i = 0; i < GVE_PRIV_FLAGS_STR_LEN; i++) {

Please remove this pointless loop.

> +		if (priv->ethtool_flags & BIT(i))
> +			ret_flags |= BIT(i);
> +	}
> +	return ret_flags;
> +}
> +
> +static int gve_set_priv_flags(struct net_device *netdev, u32 flags)
> +{
> +	struct gve_priv *priv = netdev_priv(netdev);
> +	u64 ori_flags, new_flags;
> +	u32 i;
> +
> +	ori_flags = READ_ONCE(priv->ethtool_flags);
> +	new_flags = ori_flags;
> +
> +	for (i = 0; i < GVE_PRIV_FLAGS_STR_LEN; i++) {

Ditto.

> +		if (flags & BIT(i))
> +			new_flags |= BIT(i);
> +		else
> +			new_flags &= ~(BIT(i));
> +		priv->ethtool_flags = new_flags;
> +		/* set report-stats */
> +		if (strcmp(gve_gstrings_priv_flags[i], "report-stats") == 0) {
> +			/* update the stats when user turns report-stats on */
> +			if (flags & BIT(i))
> +				gve_handle_report_stats(priv);
> +			/* zero off gve stats when report-stats turned off */
> +			if (!(flags & BIT(i)) && (ori_flags & BIT(i))) {
> +				int tx_stats_num = GVE_TX_STATS_REPORT_NUM *
> +					priv->tx_cfg.num_queues;
> +				int rx_stats_num = GVE_RX_STATS_REPORT_NUM *
> +					priv->rx_cfg.num_queues;

new line here

> +				memset(priv->stats_report->stats, 0,
> +				       (tx_stats_num + rx_stats_num) *
> +				       sizeof(struct stats));
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}


> +static int gve_alloc_stats_report(struct gve_priv *priv)
> +{
> +	int tx_stats_num, rx_stats_num;
> +
> +	tx_stats_num = (GVE_TX_STATS_REPORT_NUM) *
> +		       priv->tx_cfg.num_queues;
> +	rx_stats_num = (GVE_RX_STATS_REPORT_NUM) *
> +		       priv->rx_cfg.num_queues;
> +	priv->stats_report_len = sizeof(struct gve_stats_report) +
> +				 (tx_stats_num + rx_stats_num) *
> +				 sizeof(struct stats);
> +	priv->stats_report =
> +		dma_alloc_coherent(&priv->pdev->dev, priv->stats_report_len,
> +				   &priv->stats_report_bus, GFP_KERNEL);
> +	if (!priv->stats_report)
> +		return -ENOMEM;
> +	/* Set up timer for periodic task */
> +	timer_setup(&priv->service_timer, gve_service_timer, 0);
> +	priv->service_timer_period = GVE_SERVICE_TIMER_PERIOD;
> +	/* Start the service task timer */
> +	mod_timer(&priv->service_timer,
> +		  round_jiffies(jiffies +
> +		  msecs_to_jiffies(priv->service_timer_period)));
> +	return 0;
> +}

> @@ -1173,6 +1315,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	priv->db_bar2 = db_bar;
>  	priv->service_task_flags = 0x0;
>  	priv->state_flags = 0x0;
> +	priv->ethtool_flags = 0x0;
>  	priv->dma_mask = dma_mask;

You allocate the memory and start the timer even tho the priv flag
defaults to off?
David Awogbemila Sept. 2, 2020, 6:43 p.m. UTC | #3 
On Tue, Sep 1, 2020 at 3:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > @@ -1133,7 +1133,9 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >               goto abort_with_db_bar;
> >       }
> >       SET_NETDEV_DEV(dev, &pdev->dev);
> > +
> >       pci_set_drvdata(pdev, dev);
> > +
> >       dev->ethtool_ops = &gve_ethtool_ops;
> >       dev->netdev_ops = &gve_netdev_ops;
> >       /* advertise features */
> > @@ -1160,6 +1162,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >       priv->state_flags = 0x0;
> >
> >       gve_set_probe_in_progress(priv);
> > +
> >       priv->gve_wq = alloc_ordered_workqueue("gve", 0);
> >       if (!priv->gve_wq) {
> >               dev_err(&pdev->dev, "Could not allocate workqueue");
> > @@ -1181,6 +1184,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >       dev_info(&pdev->dev, "GVE version %s\n", gve_version_str);
> >       gve_clear_probe_in_progress(priv);
> >       queue_work(priv->gve_wq, &priv->service_task);
> > +
> >       return 0;
> >
> >  abort_with_wq:
>
> No white space changes please. If you want these, put them into a
> patch of there own.
>
>       Andrew
Thanks I'll fix this.
David Awogbemila Sept. 2, 2020, 8:39 p.m. UTC | #4 
On Tue, Sep 1, 2020 at 5:46 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue,  1 Sep 2020 14:51:45 -0700 David Awogbemila wrote:
>
> > @@ -297,6 +317,22 @@ static inline void gve_clear_probe_in_progress(struct gve_priv *priv)
> >       clear_bit(GVE_PRIV_FLAGS_PROBE_IN_PROGRESS, &priv->service_task_flags);
> >  }
> >
> > +static inline bool gve_get_do_report_stats(struct gve_priv *priv)
> > +{
> > +     return test_bit(GVE_PRIV_FLAGS_DO_REPORT_STATS,
> > +                     &priv->service_task_flags);
> > +}
> > +
> > +static inline void gve_set_do_report_stats(struct gve_priv *priv)
> > +{
> > +     set_bit(GVE_PRIV_FLAGS_DO_REPORT_STATS, &priv->service_task_flags);
> > +}
> > +
> > +static inline void gve_clear_do_report_stats(struct gve_priv *priv)
> > +{
> > +     clear_bit(GVE_PRIV_FLAGS_DO_REPORT_STATS, &priv->service_task_flags);
> > +}
> > +
> >  static inline bool gve_get_admin_queue_ok(struct gve_priv *priv)
> >  {
> >       return test_bit(GVE_PRIV_FLAGS_ADMIN_QUEUE_OK, &priv->state_flags);
> > @@ -357,6 +393,21 @@ static inline void gve_clear_napi_enabled(struct gve_priv *priv)
> >       clear_bit(GVE_PRIV_FLAGS_NAPI_ENABLED, &priv->state_flags);
> >  }
> >
> > +static inline bool gve_get_report_stats(struct gve_priv *priv)
> > +{
> > +     return test_bit(GVE_PRIV_FLAGS_REPORT_STATS, &priv->ethtool_flags);
> > +}
> > +
> > +static inline void gve_set_report_stats(struct gve_priv *priv)
>
> Please remove the unused helpers.
Thanks, I'll fix this.

>
> > +{
> > +     set_bit(GVE_PRIV_FLAGS_REPORT_STATS, &priv->ethtool_flags);
> > +}
> > +
> > +static inline void gve_clear_report_stats(struct gve_priv *priv)
> > +{
> > +     clear_bit(GVE_PRIV_FLAGS_REPORT_STATS, &priv->ethtool_flags);
> > +}
>
> > @@ -353,6 +377,54 @@ static int gve_set_tunable(struct net_device *netdev,
> >       }
> >  }
> >
> > +static u32 gve_get_priv_flags(struct net_device *netdev)
> > +{
> > +     struct gve_priv *priv = netdev_priv(netdev);
> > +     u32 i, ret_flags = 0;
> > +
> > +     for (i = 0; i < GVE_PRIV_FLAGS_STR_LEN; i++) {
>
> Please remove this pointless loop.
Thanks, I'll fix this.

>
> > +             if (priv->ethtool_flags & BIT(i))
> > +                     ret_flags |= BIT(i);
> > +     }
> > +     return ret_flags;
> > +}
> > +
> > +static int gve_set_priv_flags(struct net_device *netdev, u32 flags)
> > +{
> > +     struct gve_priv *priv = netdev_priv(netdev);
> > +     u64 ori_flags, new_flags;
> > +     u32 i;
> > +
> > +     ori_flags = READ_ONCE(priv->ethtool_flags);
> > +     new_flags = ori_flags;
> > +
> > +     for (i = 0; i < GVE_PRIV_FLAGS_STR_LEN; i++) {
>
> Ditto.
Thanks, I'll fix this.

>
> > +             if (flags & BIT(i))
> > +                     new_flags |= BIT(i);
> > +             else
> > +                     new_flags &= ~(BIT(i));
> > +             priv->ethtool_flags = new_flags;
> > +             /* set report-stats */
> > +             if (strcmp(gve_gstrings_priv_flags[i], "report-stats") == 0) {
> > +                     /* update the stats when user turns report-stats on */
> > +                     if (flags & BIT(i))
> > +                             gve_handle_report_stats(priv);
> > +                     /* zero off gve stats when report-stats turned off */
> > +                     if (!(flags & BIT(i)) && (ori_flags & BIT(i))) {
> > +                             int tx_stats_num = GVE_TX_STATS_REPORT_NUM *
> > +                                     priv->tx_cfg.num_queues;
> > +                             int rx_stats_num = GVE_RX_STATS_REPORT_NUM *
> > +                                     priv->rx_cfg.num_queues;
>
> new line here
Thanks, I'll fix this.

>
> > +                             memset(priv->stats_report->stats, 0,
> > +                                    (tx_stats_num + rx_stats_num) *
> > +                                    sizeof(struct stats));
> > +                     }
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
>
>
> > +static int gve_alloc_stats_report(struct gve_priv *priv)
> > +{
> > +     int tx_stats_num, rx_stats_num;
> > +
> > +     tx_stats_num = (GVE_TX_STATS_REPORT_NUM) *
> > +                    priv->tx_cfg.num_queues;
> > +     rx_stats_num = (GVE_RX_STATS_REPORT_NUM) *
> > +                    priv->rx_cfg.num_queues;
> > +     priv->stats_report_len = sizeof(struct gve_stats_report) +
> > +                              (tx_stats_num + rx_stats_num) *
> > +                              sizeof(struct stats);
> > +     priv->stats_report =
> > +             dma_alloc_coherent(&priv->pdev->dev, priv->stats_report_len,
> > +                                &priv->stats_report_bus, GFP_KERNEL);
> > +     if (!priv->stats_report)
> > +             return -ENOMEM;
> > +     /* Set up timer for periodic task */
> > +     timer_setup(&priv->service_timer, gve_service_timer, 0);
> > +     priv->service_timer_period = GVE_SERVICE_TIMER_PERIOD;
> > +     /* Start the service task timer */
> > +     mod_timer(&priv->service_timer,
> > +               round_jiffies(jiffies +
> > +               msecs_to_jiffies(priv->service_timer_period)));
> > +     return 0;
> > +}
>
> > @@ -1173,6 +1315,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >       priv->db_bar2 = db_bar;
> >       priv->service_task_flags = 0x0;
> >       priv->state_flags = 0x0;
> > +     priv->ethtool_flags = 0x0;
> >       priv->dma_mask = dma_mask;
>
> You allocate the memory and start the timer even tho the priv flag
> defaults to off?
That's correct. But the allocated space is still written to by the NIC
and those stats would still be available to the guest/driver.