| Message ID | 1538558230-16576-2-git-send-email-sgruszka@redhat.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [RFC,1/2] mt76: remove rx_page_lock | expand |
> +static DEFINE_PER_CPU(struct page_frag_cache, mt76_frag_cache); > + > +void *mt76_alloc_frag(unsigned int fragsz) > +{ > + struct page_frag_cache *fc; > + unsigned long flags; > + void *data; > + > + local_irq_save(flags); I like this approach since we will avoid a cache miss for the spinlock :) Do we still need to disable local_irq here since (not considering fw upload) I guess there is no contention for mt76_frag_cache Regards, Lorenzo > + fc = this_cpu_ptr(&mt76_frag_cache); > + data = page_frag_alloc(fc, fragsz, GFP_ATOMIC); > + local_irq_restore(flags); > + return data; > +} > +EXPORT_SYMBOL_GPL(mt76_alloc_frag); > + > MODULE_LICENSE("Dual BSD/GPL"); > diff --git a/drivers/net/wireless/mediatek/mt76/util.h b/drivers/net/wireless/mediatek/mt76/util.h > index 018d475504a2..6cb6c0e993c4 100644 > --- a/drivers/net/wireless/mediatek/mt76/util.h > +++ b/drivers/net/wireless/mediatek/mt76/util.h > @@ -41,4 +41,5 @@ mt76_skb_set_moredata(struct sk_buff *skb, bool enable) > hdr->frame_control &= ~cpu_to_le16(IEEE80211_FCTL_MOREDATA); > } > > +void *mt76_alloc_frag(unsigned int fragsz); > #endif > -- > 2.7.5 >
On Wed, Oct 03, 2018 at 11:38:54AM +0200, Lorenzo Bianconi wrote: > > +static DEFINE_PER_CPU(struct page_frag_cache, mt76_frag_cache); > > + > > +void *mt76_alloc_frag(unsigned int fragsz) > > +{ > > + struct page_frag_cache *fc; > > + unsigned long flags; > > + void *data; > > + > > + local_irq_save(flags); > > I like this approach since we will avoid a cache miss for the spinlock :) > Do we still need to disable local_irq here since (not considering fw upload) > I guess there is no contention for mt76_frag_cache I think is needed if we have more than one device, but I think we can change to local_irq_disable() / local_irq_enable() . Thanks Stanislaw
> On 3. Oct 2018, at 11:17, Stanislaw Gruszka <sgruszka@redhat.com> wrote: > > Make mt76 frag cache similar to netdev frag cache. This should > make frag allocation safe regarding concurrent access and also > be more efficient since we will use pages that most likely are > hot on particular cpu. > > And we don't need to clean up the cache up during device removal. The problem with feeding multiple queues with buffers from the same fragment cache is the fact that this re-introduces the problem of large compound pages staying pinned in memory (often with only one or two fragments actually being used) far too long, leading to excessive RAM usage. That is much more critical than the spinlock on alloc issue. Please keep allocation the way it is now. - Felix
diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c index f7fbd7016403..59453a7781c5 100644 --- a/drivers/net/wireless/mediatek/mt76/dma.c +++ b/drivers/net/wireless/mediatek/mt76/dma.c @@ -328,7 +328,7 @@ mt76_dma_rx_fill(struct mt76_dev *dev, struct mt76_queue *q, bool napi) while (q->queued < q->ndesc - 1) { struct mt76_queue_buf qbuf; - buf = page_frag_alloc(&q->rx_page, q->buf_size, GFP_ATOMIC); + buf = mt76_alloc_frag(q->buf_size); if (!buf) break; diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c index a103b77ae8c4..a892f59a32d8 100644 --- a/drivers/net/wireless/mediatek/mt76/usb.c +++ b/drivers/net/wireless/mediatek/mt76/usb.c @@ -276,7 +276,6 @@ static int mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf, int nsgs, int len, int sglen) { - struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN]; struct urb *urb = buf->urb; int i; @@ -285,7 +284,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf, void *data; int offset; - data = page_frag_alloc(&q->rx_page, len, GFP_ATOMIC); + data = mt76_alloc_frag(len); if (!data) break; @@ -557,13 +556,6 @@ static void mt76u_free_rx(struct mt76_dev *dev) for (i = 0; i < q->ndesc; i++) mt76u_buf_free(&q->entry[i].ubuf); - - if (!q->rx_page.va) - return; - - page = virt_to_page(q->rx_page.va); - __page_frag_cache_drain(page, q->rx_page.pagecnt_bias); - memset(&q->rx_page, 0, sizeof(q->rx_page)); } static void mt76u_stop_rx(struct mt76_dev *dev) diff --git a/drivers/net/wireless/mediatek/mt76/util.c b/drivers/net/wireless/mediatek/mt76/util.c index 0c35b8db58cd..def2a1b841b9 100644 --- a/drivers/net/wireless/mediatek/mt76/util.c +++ b/drivers/net/wireless/mediatek/mt76/util.c @@ -75,4 +75,20 @@ int mt76_wcid_alloc(unsigned long *mask, int size) } EXPORT_SYMBOL_GPL(mt76_wcid_alloc); +static DEFINE_PER_CPU(struct page_frag_cache, mt76_frag_cache); + +void *mt76_alloc_frag(unsigned int fragsz) +{ + struct page_frag_cache *fc; + unsigned long flags; + void *data; + + local_irq_save(flags); + fc = this_cpu_ptr(&mt76_frag_cache); + data = page_frag_alloc(fc, fragsz, GFP_ATOMIC); + local_irq_restore(flags); + return data; +} +EXPORT_SYMBOL_GPL(mt76_alloc_frag); + MODULE_LICENSE("Dual BSD/GPL"); diff --git a/drivers/net/wireless/mediatek/mt76/util.h b/drivers/net/wireless/mediatek/mt76/util.h index 018d475504a2..6cb6c0e993c4 100644 --- a/drivers/net/wireless/mediatek/mt76/util.h +++ b/drivers/net/wireless/mediatek/mt76/util.h @@ -41,4 +41,5 @@ mt76_skb_set_moredata(struct sk_buff *skb, bool enable) hdr->frame_control &= ~cpu_to_le16(IEEE80211_FCTL_MOREDATA); } +void *mt76_alloc_frag(unsigned int fragsz); #endif
Make mt76 frag cache similar to netdev frag cache. This should make frag allocation safe regarding concurrent access and also be more efficient since we will use pages that most likely are hot on particular cpu. And we don't need to clean up the cache up during device removal. Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> --- drivers/net/wireless/mediatek/mt76/dma.c | 2 +- drivers/net/wireless/mediatek/mt76/usb.c | 10 +--------- drivers/net/wireless/mediatek/mt76/util.c | 16 ++++++++++++++++ drivers/net/wireless/mediatek/mt76/util.h | 1 + 4 files changed, 19 insertions(+), 10 deletions(-)