From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9803AC3A5A6 for ; Mon, 23 Sep 2019 09:49:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 67F1F20835 for ; Mon, 23 Sep 2019 09:49:07 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="IKBos90G"; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="nj4BAorw" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2407286AbfIWJtG (ORCPT ); Mon, 23 Sep 2019 05:49:06 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:49436 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2407268AbfIWJtG (ORCPT ); Mon, 23 Sep 2019 05:49:06 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 4E38660274; Mon, 23 Sep 2019 09:49:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1569232145; bh=l+G9gwJkMnxph70rqdX1rD9rOffkiVc7oJo/k+NvESY=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=IKBos90GHXlfK2JuiZhirbfaklsQ+qGaunjJ0dRZdPY9QQG9nJgQULBiO6B3W03fN X2/UQ8cyc02HB2im3bK+v6OGGJNUM1i2L99ZNXpfXgaU+GXTfe4Ez1jvOdQ0FMjS6R ovbHceBHgPZk3RQ/MP1tIh8kgoxiroiVBiSVrKWk= Received: from x230.qca.qualcomm.com (37-136-106-186.rev.dnainternet.fi [37.136.106.186]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: kvalo@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 7ED366021C; Mon, 23 Sep 2019 09:49:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1569232144; bh=l+G9gwJkMnxph70rqdX1rD9rOffkiVc7oJo/k+NvESY=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=nj4BAorwAPI+DHUjmC4Sd4Cob+CL+rJN5/YXon3Pk+95rbDvrA0R9Ofy25UScVB99 H1Yq/Vgc0CmQIDveNyGSIKZg/8MXLw50zFfd2Bnixo7vt6/6VOZIW0G7oLbokX4HWj Jx/mJsF9sUUYPQ8V9RDbO2+AhgVbs7GLADVQNtTw= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 7ED366021C Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=kvalo@codeaurora.org From: Kalle Valo To: Wen Gong Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org Subject: Re: [PATCH v5 4/8] ath10k: add workqueue for RX path of sdio References: <1567679893-14029-1-git-send-email-wgong@codeaurora.org> <1567679893-14029-5-git-send-email-wgong@codeaurora.org> Date: Mon, 23 Sep 2019 12:49:01 +0300 In-Reply-To: <1567679893-14029-5-git-send-email-wgong@codeaurora.org> (Wen Gong's message of "Thu, 5 Sep 2019 18:38:09 +0800") Message-ID: <87o8zb8hrm.fsf@codeaurora.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org Wen Gong writes: > The thread of read rx message by sdio bus from firmware is > synchronous, it will cost much time for process the left part > of rx message which includes indicate the rx packet to uppper > net stack. It will reduce the time of read from sdio. This paragraph is hard to read. > This patch move the indication to a workqueue, it results in > significant performance improvement on RX path. How much is "significant"? Some numbers would make it a lot easier to understand the importance of the changes. > Tested with QCA6174 SDIO with firmware > WLAN.RMH.4.4.1-00007-QCARMSWP-1. > > Signed-off-by: Wen Gong [...] > +static struct ath10k_sdio_rx_request *ath10k_sdio_alloc_rx_req(struct ath10k *ar) > +{ > + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar); > + struct ath10k_sdio_rx_request *rx_req; > + > + spin_lock_bh(&ar_sdio->rx_lock); > + > + if (list_empty(&ar_sdio->rx_req_freeq)) { > + rx_req = NULL; > + ath10k_dbg(ar, ATH10K_DBG_SDIO, "rx_req alloc fail\n"); > + goto out; > + } > + > + rx_req = list_first_entry(&ar_sdio->rx_req_freeq, > + struct ath10k_sdio_rx_request, list); > + list_del(&rx_req->list); > + > +out: > + spin_unlock_bh(&ar_sdio->rx_lock); > + return rx_req; > +} The name of the function is "alloc" but it does not allocate anything? > +static void ath10k_sdio_free_rx_req(struct ath10k *ar, > + struct ath10k_sdio_rx_request *rx_req) > +{ > + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar); > + > + memset(rx_req, 0, sizeof(*rx_req)); > + > + spin_lock_bh(&ar_sdio->rx_lock); > + list_add_tail(&rx_req->list, &ar_sdio->rx_req_freeq); > + spin_unlock_bh(&ar_sdio->rx_lock); > +} And this neither frees anything. IMHO that's quite misleading naming. Maybe call _get() and _put()? Or maybe there's some more common naming in kernel? > +static int ath10k_sdio_prep_async_rx_req(struct ath10k *ar, > + struct sk_buff *skb, > + struct ath10k_htc_ep *ep) > +{ > + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar); > + struct ath10k_sdio_rx_request *rx_req; > + > + /* Allocate a rx request for the message and queue it on the > + * SDIO rx workqueue. > + */ > + rx_req = ath10k_sdio_alloc_rx_req(ar); > + if (!rx_req) { > + ath10k_warn(ar, "unable to allocate rx request for async request\n"); > + return -ENOMEM; > + } > + > + rx_req->skb = skb; > + rx_req->ep = ep; > + > + spin_lock_bh(&ar_sdio->wr_async_lock_rx); > + list_add_tail(&rx_req->list, &ar_sdio->wr_asyncq_rx); > + spin_unlock_bh(&ar_sdio->wr_async_lock_rx); > + > + return 0; > +} Is there enough room in struct ath10k_skb_cb so that you could add struct ath10k_htc_ep there? That way struct ath10k_sdio_rx_request would be useless and you could just use skb_queue_*() functions, which would make this patch a lot simpler. > @@ -209,6 +224,11 @@ struct ath10k_sdio { > struct list_head wr_asyncq; > /* protects access to wr_asyncq */ > spinlock_t wr_async_lock; > + > + struct work_struct wr_async_work_rx; > + struct list_head wr_asyncq_rx; > + /* protects access to wr_asyncq_rx */ > + spinlock_t wr_async_lock_rx; > }; I think the naming is now confusing. I'm guessing "wr_async_lock" means "write asynchronous lock"? So what is wr_asyncq_rx then? It would a lot simpler if we have tx_lock and rx_lock, or something like that. Naturally all cleanup for wr_async_lock needs to be done in a separate patch. -- Kalle Valo From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]) by bombadil.infradead.org with esmtps (Exim 4.92.2 #3 (Red Hat Linux)) id 1iCKxt-0005cv-De for ath10k@lists.infradead.org; Mon, 23 Sep 2019 09:49:06 +0000 From: Kalle Valo Subject: Re: [PATCH v5 4/8] ath10k: add workqueue for RX path of sdio References: <1567679893-14029-1-git-send-email-wgong@codeaurora.org> <1567679893-14029-5-git-send-email-wgong@codeaurora.org> Date: Mon, 23 Sep 2019 12:49:01 +0300 In-Reply-To: <1567679893-14029-5-git-send-email-wgong@codeaurora.org> (Wen Gong's message of "Thu, 5 Sep 2019 18:38:09 +0800") Message-ID: <87o8zb8hrm.fsf@codeaurora.org> MIME-Version: 1.0 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "ath10k" Errors-To: ath10k-bounces+kvalo=adurom.com@lists.infradead.org To: Wen Gong Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org Wen Gong writes: > The thread of read rx message by sdio bus from firmware is > synchronous, it will cost much time for process the left part > of rx message which includes indicate the rx packet to uppper > net stack. It will reduce the time of read from sdio. This paragraph is hard to read. > This patch move the indication to a workqueue, it results in > significant performance improvement on RX path. How much is "significant"? Some numbers would make it a lot easier to understand the importance of the changes. > Tested with QCA6174 SDIO with firmware > WLAN.RMH.4.4.1-00007-QCARMSWP-1. > > Signed-off-by: Wen Gong [...] > +static struct ath10k_sdio_rx_request *ath10k_sdio_alloc_rx_req(struct ath10k *ar) > +{ > + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar); > + struct ath10k_sdio_rx_request *rx_req; > + > + spin_lock_bh(&ar_sdio->rx_lock); > + > + if (list_empty(&ar_sdio->rx_req_freeq)) { > + rx_req = NULL; > + ath10k_dbg(ar, ATH10K_DBG_SDIO, "rx_req alloc fail\n"); > + goto out; > + } > + > + rx_req = list_first_entry(&ar_sdio->rx_req_freeq, > + struct ath10k_sdio_rx_request, list); > + list_del(&rx_req->list); > + > +out: > + spin_unlock_bh(&ar_sdio->rx_lock); > + return rx_req; > +} The name of the function is "alloc" but it does not allocate anything? > +static void ath10k_sdio_free_rx_req(struct ath10k *ar, > + struct ath10k_sdio_rx_request *rx_req) > +{ > + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar); > + > + memset(rx_req, 0, sizeof(*rx_req)); > + > + spin_lock_bh(&ar_sdio->rx_lock); > + list_add_tail(&rx_req->list, &ar_sdio->rx_req_freeq); > + spin_unlock_bh(&ar_sdio->rx_lock); > +} And this neither frees anything. IMHO that's quite misleading naming. Maybe call _get() and _put()? Or maybe there's some more common naming in kernel? > +static int ath10k_sdio_prep_async_rx_req(struct ath10k *ar, > + struct sk_buff *skb, > + struct ath10k_htc_ep *ep) > +{ > + struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar); > + struct ath10k_sdio_rx_request *rx_req; > + > + /* Allocate a rx request for the message and queue it on the > + * SDIO rx workqueue. > + */ > + rx_req = ath10k_sdio_alloc_rx_req(ar); > + if (!rx_req) { > + ath10k_warn(ar, "unable to allocate rx request for async request\n"); > + return -ENOMEM; > + } > + > + rx_req->skb = skb; > + rx_req->ep = ep; > + > + spin_lock_bh(&ar_sdio->wr_async_lock_rx); > + list_add_tail(&rx_req->list, &ar_sdio->wr_asyncq_rx); > + spin_unlock_bh(&ar_sdio->wr_async_lock_rx); > + > + return 0; > +} Is there enough room in struct ath10k_skb_cb so that you could add struct ath10k_htc_ep there? That way struct ath10k_sdio_rx_request would be useless and you could just use skb_queue_*() functions, which would make this patch a lot simpler. > @@ -209,6 +224,11 @@ struct ath10k_sdio { > struct list_head wr_asyncq; > /* protects access to wr_asyncq */ > spinlock_t wr_async_lock; > + > + struct work_struct wr_async_work_rx; > + struct list_head wr_asyncq_rx; > + /* protects access to wr_asyncq_rx */ > + spinlock_t wr_async_lock_rx; > }; I think the naming is now confusing. I'm guessing "wr_async_lock" means "write asynchronous lock"? So what is wr_asyncq_rx then? It would a lot simpler if we have tx_lock and rx_lock, or something like that. Naturally all cleanup for wr_async_lock needs to be done in a separate patch. -- Kalle Valo _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k