Re: [PATCH net 2/4] ibmvnic: init ->running_cap_crqs early

[Dany Madden <drt@linux.ibm.com>]

On 2022-01-21 18:59, Sukadev Bhattiprolu wrote:
> We use ->running_cap_crqs to determine when the ibmvnic_tasklet() 
> should
> send out the next protocol message type. i.e when we get back responses
> to all our QUERY_CAPABILITY CRQs we send out REQUEST_CAPABILITY crqs.
> Similiary, when we get responses to all the REQUEST_CAPABILITY crqs, we
> send out the QUERY_IP_OFFLOAD CRQ.
> 
> We currently increment ->running_cap_crqs as we send out each CRQ and
> have the ibmvnic_tasklet() send out the next message type, when this
> running_cap_crqs count drops to 0.
> 
> This assumes that all the CRQs of the current type were sent out before
> the count drops to 0. However it is possible that we send out say 6 
> CRQs,
> get preempted and receive all the 6 responses before we send out the
> remaining CRQs. This can result in ->running_cap_crqs count dropping to
> zero before all messages of the current type were sent and we end up
> sending the next protocol message too early.
> 
> Instead initialize the ->running_cap_crqs upfront so the tasklet will
> only send the next protocol message after all responses are received.
> 
> Use the cap_reqs local variable to also detect any discrepancy (either
> now or in future) in the number of capability requests we actually 
> send.
> 
> Currently only send_query_cap() is affected by this behavior (of 
> sending
> next message early) since it is called from the worker thread (during
> reset) and from application thread (during ->ndo_open()) and they can 
> be
> preempted. send_request_cap() is only called from the tasklet  which
> processes CRQ responses sequentially, is not be affected.  But to
> maintain the existing symmtery with send_query_capability() we update
> send_request_capability() also.
> 
> Fixes: 249168ad07cd ("ibmvnic: Make CRQ interrupt tasklet wait for all
> capabilities crqs")
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
Reviewed-by: Dany Madden <drt@linux.ibm.com>

> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 106 +++++++++++++++++++----------
>  1 file changed, 71 insertions(+), 35 deletions(-)
> 
> 
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index 9b2d16ad76f1..acd488310bbc 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -3849,11 +3849,25 @@ static void send_request_cap(struct
> ibmvnic_adapter *adapter, int retry)
>  	struct device *dev = &adapter->vdev->dev;
>  	union ibmvnic_crq crq;
>  	int max_entries;
> +	int cap_reqs;
> +
> +	/* We send out 6 or 7 REQUEST_CAPABILITY CRQs below (depending on
> +	 * the PROMISC flag). Initialize this count upfront. When the tasklet
> +	 * receives a response to all of these, it will send the next 
> protocol
> +	 * message (QUERY_IP_OFFLOAD).
> +	 */
> +	if (!(adapter->netdev->flags & IFF_PROMISC) ||
> +	    adapter->promisc_supported)
> +		cap_reqs = 7;
> +	else
> +		cap_reqs = 6;
> 
>  	if (!retry) {
>  		/* Sub-CRQ entries are 32 byte long */
>  		int entries_page = 4 * PAGE_SIZE / (sizeof(u64) * 4);
> 
> +		atomic_set(&adapter->running_cap_crqs, cap_reqs);
> +
>  		if (adapter->min_tx_entries_per_subcrq > entries_page ||
>  		    adapter->min_rx_add_entries_per_subcrq > entries_page) {
>  			dev_err(dev, "Fatal, invalid entries per sub-crq\n");
> @@ -3914,44 +3928,45 @@ static void send_request_cap(struct
> ibmvnic_adapter *adapter, int retry)
>  					adapter->opt_rx_comp_queues;
> 
>  		adapter->req_rx_add_queues = adapter->max_rx_add_queues;
> +	} else {
> +		atomic_add(cap_reqs, &adapter->running_cap_crqs);
>  	}
> -
>  	memset(&crq, 0, sizeof(crq));
>  	crq.request_capability.first = IBMVNIC_CRQ_CMD;
>  	crq.request_capability.cmd = REQUEST_CAPABILITY;
> 
>  	crq.request_capability.capability = cpu_to_be16(REQ_TX_QUEUES);
>  	crq.request_capability.number = cpu_to_be64(adapter->req_tx_queues);
> -	atomic_inc(&adapter->running_cap_crqs);
> +	cap_reqs--;
>  	ibmvnic_send_crq(adapter, &crq);
> 
>  	crq.request_capability.capability = cpu_to_be16(REQ_RX_QUEUES);
>  	crq.request_capability.number = cpu_to_be64(adapter->req_rx_queues);
> -	atomic_inc(&adapter->running_cap_crqs);
> +	cap_reqs--;
>  	ibmvnic_send_crq(adapter, &crq);
> 
>  	crq.request_capability.capability = cpu_to_be16(REQ_RX_ADD_QUEUES);
>  	crq.request_capability.number = 
> cpu_to_be64(adapter->req_rx_add_queues);
> -	atomic_inc(&adapter->running_cap_crqs);
> +	cap_reqs--;
>  	ibmvnic_send_crq(adapter, &crq);
> 
>  	crq.request_capability.capability =
>  	    cpu_to_be16(REQ_TX_ENTRIES_PER_SUBCRQ);
>  	crq.request_capability.number =
>  	    cpu_to_be64(adapter->req_tx_entries_per_subcrq);
> -	atomic_inc(&adapter->running_cap_crqs);
> +	cap_reqs--;
>  	ibmvnic_send_crq(adapter, &crq);
> 
>  	crq.request_capability.capability =
>  	    cpu_to_be16(REQ_RX_ADD_ENTRIES_PER_SUBCRQ);
>  	crq.request_capability.number =
>  	    cpu_to_be64(adapter->req_rx_add_entries_per_subcrq);
> -	atomic_inc(&adapter->running_cap_crqs);
> +	cap_reqs--;
>  	ibmvnic_send_crq(adapter, &crq);
> 
>  	crq.request_capability.capability = cpu_to_be16(REQ_MTU);
>  	crq.request_capability.number = cpu_to_be64(adapter->req_mtu);
> -	atomic_inc(&adapter->running_cap_crqs);
> +	cap_reqs--;
>  	ibmvnic_send_crq(adapter, &crq);
> 
>  	if (adapter->netdev->flags & IFF_PROMISC) {
> @@ -3959,16 +3974,21 @@ static void send_request_cap(struct
> ibmvnic_adapter *adapter, int retry)
>  			crq.request_capability.capability =
>  			    cpu_to_be16(PROMISC_REQUESTED);
>  			crq.request_capability.number = cpu_to_be64(1);
> -			atomic_inc(&adapter->running_cap_crqs);
> +			cap_reqs--;
>  			ibmvnic_send_crq(adapter, &crq);
>  		}
>  	} else {
>  		crq.request_capability.capability =
>  		    cpu_to_be16(PROMISC_REQUESTED);
>  		crq.request_capability.number = cpu_to_be64(0);
> -		atomic_inc(&adapter->running_cap_crqs);
> +		cap_reqs--;
>  		ibmvnic_send_crq(adapter, &crq);
>  	}
> +
> +	/* Keep at end to catch any discrepancy between expected and actual
> +	 * CRQs sent.
> +	 */
> +	WARN_ON(cap_reqs != 0);
>  }
> 
>  static int pending_scrq(struct ibmvnic_adapter *adapter,
> @@ -4362,118 +4382,132 @@ static void send_query_map(struct
> ibmvnic_adapter *adapter)
>  static void send_query_cap(struct ibmvnic_adapter *adapter)
>  {
>  	union ibmvnic_crq crq;
> +	int cap_reqs;
> +
> +	/* We send out 25 QUERY_CAPABILITY CRQs below.  Initialize this count
> +	 * upfront. When the tasklet receives a response to all of these, it
> +	 * can send out the next protocol messaage (REQUEST_CAPABILITY).
> +	 */
> +	cap_reqs = 25;
> +
> +	atomic_set(&adapter->running_cap_crqs, cap_reqs);
> 
> -	atomic_set(&adapter->running_cap_crqs, 0);
>  	memset(&crq, 0, sizeof(crq));
>  	crq.query_capability.first = IBMVNIC_CRQ_CMD;
>  	crq.query_capability.cmd = QUERY_CAPABILITY;
> 
>  	crq.query_capability.capability = cpu_to_be16(MIN_TX_QUEUES);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(MIN_RX_QUEUES);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(MIN_RX_ADD_QUEUES);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(MAX_TX_QUEUES);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(MAX_RX_QUEUES);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(MAX_RX_ADD_QUEUES);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability =
>  	    cpu_to_be16(MIN_TX_ENTRIES_PER_SUBCRQ);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability =
>  	    cpu_to_be16(MIN_RX_ADD_ENTRIES_PER_SUBCRQ);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability =
>  	    cpu_to_be16(MAX_TX_ENTRIES_PER_SUBCRQ);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability =
>  	    cpu_to_be16(MAX_RX_ADD_ENTRIES_PER_SUBCRQ);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(TCP_IP_OFFLOAD);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(PROMISC_SUPPORTED);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(MIN_MTU);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(MAX_MTU);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(MAX_MULTICAST_FILTERS);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(VLAN_HEADER_INSERTION);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = 
> cpu_to_be16(RX_VLAN_HEADER_INSERTION);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(MAX_TX_SG_ENTRIES);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(RX_SG_SUPPORTED);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = 
> cpu_to_be16(OPT_TX_COMP_SUB_QUEUES);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(OPT_RX_COMP_QUEUES);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability =
>  			cpu_to_be16(OPT_RX_BUFADD_Q_PER_RX_COMP_Q);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability =
>  			cpu_to_be16(OPT_TX_ENTRIES_PER_SUBCRQ);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability =
>  			cpu_to_be16(OPT_RXBA_ENTRIES_PER_SUBCRQ);
> -	atomic_inc(&adapter->running_cap_crqs);
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> 
>  	crq.query_capability.capability = cpu_to_be16(TX_RX_DESC_REQ);
> -	atomic_inc(&adapter->running_cap_crqs);
> +
>  	ibmvnic_send_crq(adapter, &crq);
> +	cap_reqs--;
> +
> +	/* Keep at end to catch any discrepancy between expected and actual
> +	 * CRQs sent.
> +	 */
> +	WARN_ON(cap_reqs != 0);
>  }
> 
>  static void send_query_ip_offload(struct ibmvnic_adapter *adapter)
> @@ -4777,6 +4811,8 @@ static void handle_request_cap_rsp(union 
> ibmvnic_crq *crq,
>  	char *name;
> 
>  	atomic_dec(&adapter->running_cap_crqs);
> +	netdev_dbg(adapter->netdev, "Outstanding request-caps: %d\n",
> +		   atomic_read(&adapter->running_cap_crqs));
>  	switch (be16_to_cpu(crq->request_capability_rsp.capability)) {
>  	case REQ_TX_QUEUES:
>  		req_value = &adapter->req_tx_queues;