netdev - Re: [PATCH net-next v2 5/9] gve: Add Gvnic stats AQ command and ethtool show/set-priv-flags.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200901174558.1745ad28@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 1 Sep 2020 17:45:58 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     David Awogbemila <awogbemila@...gle.com>
Cc:     netdev@...r.kernel.org, Kuo Zhao <kuozhao@...gle.com>,
        Yangchun Fu <yangchun@...gle.com>
Subject: Re: [PATCH net-next v2 5/9] gve: Add Gvnic stats AQ command and
 ethtool show/set-priv-flags.

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?