| Message ID | 20220517064821.3966990-1-william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: wwan: t7xx: fix GFP_KERNEL usage in spin_lock context | expand |
On Tue, May 17, 2022 at 9:30 AM Ziyang Xuan <william.xuanziyang@huawei.com> wrote: > t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock > context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses > GFP_KERNEL, that will introduce scheduling factor in spin_lock context. > > Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can > remove the spin_lock from t7xx_cldma_clear_rxq(). > > Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Hi Ziyang, On Tue, 17 May 2022 at 08:30, Ziyang Xuan <william.xuanziyang@huawei.com> wrote: > > t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock > context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses > GFP_KERNEL, that will introduce scheduling factor in spin_lock context. > > Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can > remove the spin_lock from t7xx_cldma_clear_rxq(). > > Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > --- You should normally indicate what changed in this v2. > drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c > index 46066dcd2607..7493285a9606 100644 > --- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c > +++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c > @@ -782,10 +782,12 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) > struct cldma_queue *rxq = &md_ctrl->rxq[qnum]; > struct cldma_request *req; > struct cldma_gpd *gpd; > - unsigned long flags; > int ret = 0; > > - spin_lock_irqsave(&rxq->ring_lock, flags); > + /* CLDMA has been stopped. There is not any CLDMA IRQ, holding > + * ring_lock is not needed. If it makes sense to explain why we don't need locking, the next sentence is not needed: > Thus we can use functions that may > + * introduce scheduling. > + */ > t7xx_cldma_q_reset(rxq); > list_for_each_entry(req, &rxq->tr_ring->gpd_ring, entry) { > gpd = req->gpd; > @@ -808,7 +810,6 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) > > t7xx_cldma_gpd_set_data_ptr(req->gpd, req->mapped_buff); > } > - spin_unlock_irqrestore(&rxq->ring_lock, flags); > > return ret; > } > -- > 2.25.1 >
> Hi Ziyang, > > On Tue, 17 May 2022 at 08:30, Ziyang Xuan <william.xuanziyang@huawei.com> wrote: >> >> t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock >> context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses >> GFP_KERNEL, that will introduce scheduling factor in spin_lock context. >> >> Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can >> remove the spin_lock from t7xx_cldma_clear_rxq(). >> >> Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >> --- > > You should normally indicate what changed in this v2. > >> drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c >> index 46066dcd2607..7493285a9606 100644 >> --- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c >> +++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c >> @@ -782,10 +782,12 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) >> struct cldma_queue *rxq = &md_ctrl->rxq[qnum]; >> struct cldma_request *req; >> struct cldma_gpd *gpd; >> - unsigned long flags; >> int ret = 0; >> >> - spin_lock_irqsave(&rxq->ring_lock, flags); >> + /* CLDMA has been stopped. There is not any CLDMA IRQ, holding >> + * ring_lock is not needed. > > If it makes sense to explain why we don't need locking, the next > sentence is not needed: I want to remind the possible developer if he or she want to add spin_lock here again in future, he or she should check whether there is a scheduling factor or not here firstly. > > >> Thus we can use functions that may >> + * introduce scheduling. >> + */ >> t7xx_cldma_q_reset(rxq); >> list_for_each_entry(req, &rxq->tr_ring->gpd_ring, entry) { >> gpd = req->gpd; >> @@ -808,7 +810,6 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) >> >> t7xx_cldma_gpd_set_data_ptr(req->gpd, req->mapped_buff); >> } >> - spin_unlock_irqrestore(&rxq->ring_lock, flags); >> >> return ret; >> } >> -- >> 2.25.1 >> > . >
On Tue, 17 May 2022, Ziyang Xuan wrote: > t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock > context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses > GFP_KERNEL, that will introduce scheduling factor in spin_lock context. > > Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can > remove the spin_lock from t7xx_cldma_clear_rxq(). > Perhaps Suggested-by: ... would have been appropriate too. > Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> On Tue, 17 May 2022, Ziyang Xuan wrote: > >> t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock >> context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses >> GFP_KERNEL, that will introduce scheduling factor in spin_lock context. >> >> Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can >> remove the spin_lock from t7xx_cldma_clear_rxq(). >> > > Perhaps Suggested-by: ... would have been appropriate too. Yes，I will send the v3 patch. > >> Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c index 46066dcd2607..7493285a9606 100644 --- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c +++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c @@ -782,10 +782,12 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) struct cldma_queue *rxq = &md_ctrl->rxq[qnum]; struct cldma_request *req; struct cldma_gpd *gpd; - unsigned long flags; int ret = 0; - spin_lock_irqsave(&rxq->ring_lock, flags); + /* CLDMA has been stopped. There is not any CLDMA IRQ, holding + * ring_lock is not needed. Thus we can use functions that may + * introduce scheduling. + */ t7xx_cldma_q_reset(rxq); list_for_each_entry(req, &rxq->tr_ring->gpd_ring, entry) { gpd = req->gpd; @@ -808,7 +810,6 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) t7xx_cldma_gpd_set_data_ptr(req->gpd, req->mapped_buff); } - spin_unlock_irqrestore(&rxq->ring_lock, flags); return ret; }
t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses GFP_KERNEL, that will introduce scheduling factor in spin_lock context. Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can remove the spin_lock from t7xx_cldma_clear_rxq(). Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)