Re: [PATCH net-next 10/10] ice: simplify ice_vc_dis_qs_msg() a little

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message misdescribes the code it removes.

--

ice: simplify ice_vc_dis_qs_msg() a little

This commit removes a special case in ice_vc_dis_qs_msg(). When the
requested rx_queues bitmap matched vf->rxq_ena, that case disabled all Rx
queues as a batch through ice_vsi_stop_all_rx_rings(). All Rx queues now go
through the per-queue ice_vsi_ctrl_one_rx_ring() path.

[ ... ]

[Severity: Low]
Does the commit message describe the removed path correctly? It says:

  There were no actual speedup from it, the only difference was in saved
  bitmap checks, but the HW operations take the majority of time anyway.
  ice_vsi_stop_all_rx_rings() used (in the removed code) loops over rings
  anyway.

ice_vsi_stop_all_rx_rings() ends up in ice_vsi_ctrl_all_rx_rings(). That
function sends the disable request to every ring without waiting, flushes
once, and only then polls each ring:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_ctrl_all_rx_rings() {
    ...
	ice_for_each_rxq(vsi, i)
		ice_vsi_ctrl_one_rx_ring(vsi, ena, i, false);

	ice_flush(&vsi->back->hw);

	ice_for_each_rxq(vsi, i) {
		ret = ice_vsi_wait_one_rx_ring(vsi, ena, i);
    ...
}

Its kernel-doc says this is on purpose: "This will let all of the register
writes complete when enabling/disabling the Rx rings before waiting for
the change in hardware to complete."

The path that remains calls ice_vsi_ctrl_one_rx_ring() with wait set to
true for each queue. Each call does its own ice_flush() and
ice_pf_rxq_wait() poll before the next queue starts:

drivers/net/ethernet/intel/ice/ice_base.c:ice_vsi_ctrl_one_rx_ring() {
    ...
	if (!wait)
		return 0;

	ice_flush(hw);
	return ice_pf_rxq_wait(pf, pf_q, ena);
}

So the removed code seems to have saved overlapped hardware wait time, not
bitmap checks.

ICE_MAX_RSS_QS_PER_VF is 16, and ice_pf_rxq_wait() is bounded by
ICE_Q_WAIT_MAX_RETRY polls of usleep_range(20, 40). That puts the worst
case at about 32ms instead of about 2ms. This is small and fits the
measurement in the commit message.

Could the justification be reworded to match what the code does?

The commit message also leaves out two small behaviour differences:

- The old path stopped every ring the VSI owns through ice_for_each_rxq(),
  not only the bits set in vf->rxq_ena.

- On a partial failure, the old path left vf->rxq_ena and the queue
  interrupts untouched. The per-queue path will already have cleared the
  rxq_ena bits and called ice_vf_dis_rxq_interrupt() for every queue it
  finished before the one that failed.

Should these be mentioned?

 		for_each_set_bit(vf_q_id, &q_map, ICE_MAX_RSS_QS_PER_VF) {
 			if (!ice_vc_isvalid_q_id(vsi, vf_q_id)) {
 				v_ret = VIRTCHNL_STATUS_ERR_PARAM;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929224153.1455466-1-anthony.l.nguyen%40intel.com