[PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Yunsheng Lin]

Currently the page_frag API is returning 'virtual address'
or 'va' when allocing and expecting 'virtual address' or
'va' as input when freeing.

As we are about to support new use cases that the caller
need to deal with 'struct page' or need to deal with both
'va' and 'struct page'. In order to differentiate the API
handling between 'va' and 'struct page', add '_va' suffix
to the corresponding API mirroring the page_pool_alloc_va()
API of the page_pool. So that callers expecting to deal with
va, page or both va and page may call page_frag_alloc_va*,
page_frag_alloc_pg*, or page_frag_alloc* API accordingly.

CC: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
Reviewed-by: Subbaraya Sundeep <sbhatta@marvell.com>
---
 drivers/net/ethernet/google/gve/gve_rx.c      |  4 ++--
 drivers/net/ethernet/intel/ice/ice_txrx.c     |  2 +-
 drivers/net/ethernet/intel/ice/ice_txrx.h     |  2 +-
 drivers/net/ethernet/intel/ice/ice_txrx_lib.c |  2 +-
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |  4 ++--
 .../marvell/octeontx2/nic/otx2_common.c       |  2 +-
 drivers/net/ethernet/mediatek/mtk_wed_wo.c    |  4 ++--
 drivers/nvme/host/tcp.c                       |  8 +++----
 drivers/nvme/target/tcp.c                     | 22 +++++++++----------
 drivers/vhost/net.c                           |  6 ++---
 include/linux/page_frag_cache.h               | 21 +++++++++---------
 include/linux/skbuff.h                        |  2 +-
 kernel/bpf/cpumap.c                           |  2 +-
 mm/page_frag_cache.c                          | 12 +++++-----
 mm/page_frag_test.c                           | 13 ++++++-----
 net/core/skbuff.c                             | 14 ++++++------
 net/core/xdp.c                                |  2 +-
 net/rxrpc/txbuf.c                             | 15 +++++++------
 net/sunrpc/svcsock.c                          |  6 ++---
 19 files changed, 74 insertions(+), 69 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_rx.c b/drivers/net/ethernet/google/gve/gve_rx.c
index acb73d4d0de6..b6c10100e462 100644
--- a/drivers/net/ethernet/google/gve/gve_rx.c
+++ b/drivers/net/ethernet/google/gve/gve_rx.c
@@ -729,7 +729,7 @@ static int gve_xdp_redirect(struct net_device *dev, struct gve_rx_ring *rx,
 
 	total_len = headroom + SKB_DATA_ALIGN(len) +
 		SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
-	frame = page_frag_alloc(&rx->page_cache, total_len, GFP_ATOMIC);
+	frame = page_frag_alloc_va(&rx->page_cache, total_len, GFP_ATOMIC);
 	if (!frame) {
 		u64_stats_update_begin(&rx->statss);
 		rx->xdp_alloc_fails++;
@@ -742,7 +742,7 @@ static int gve_xdp_redirect(struct net_device *dev, struct gve_rx_ring *rx,
 
 	err = xdp_do_redirect(dev, &new, xdp_prog);
 	if (err)
-		page_frag_free(frame);
+		page_frag_free_va(frame);
 
 	return err;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 8bb743f78fcb..399b317c509d 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -126,7 +126,7 @@ ice_unmap_and_free_tx_buf(struct ice_tx_ring *ring, struct ice_tx_buf *tx_buf)
 		dev_kfree_skb_any(tx_buf->skb);
 		break;
 	case ICE_TX_BUF_XDP_TX:
-		page_frag_free(tx_buf->raw_buf);
+		page_frag_free_va(tx_buf->raw_buf);
 		break;
 	case ICE_TX_BUF_XDP_XMIT:
 		xdp_return_frame(tx_buf->xdpf);
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
index feba314a3fe4..6379f57d8228 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -148,7 +148,7 @@ static inline int ice_skb_pad(void)
  * @ICE_TX_BUF_DUMMY: dummy Flow Director packet, unmap and kfree()
  * @ICE_TX_BUF_FRAG: mapped skb OR &xdp_buff frag, only unmap DMA
  * @ICE_TX_BUF_SKB: &sk_buff, unmap and consume_skb(), update stats
- * @ICE_TX_BUF_XDP_TX: &xdp_buff, unmap and page_frag_free(), stats
+ * @ICE_TX_BUF_XDP_TX: &xdp_buff, unmap and page_frag_free_va(), stats
  * @ICE_TX_BUF_XDP_XMIT: &xdp_frame, unmap and xdp_return_frame(), stats
  * @ICE_TX_BUF_XSK_TX: &xdp_buff on XSk queue, xsk_buff_free(), stats
  */
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
index 2719f0e20933..a1a41a14df0d 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
@@ -250,7 +250,7 @@ ice_clean_xdp_tx_buf(struct device *dev, struct ice_tx_buf *tx_buf,
 
 	switch (tx_buf->type) {
 	case ICE_TX_BUF_XDP_TX:
-		page_frag_free(tx_buf->raw_buf);
+		page_frag_free_va(tx_buf->raw_buf);
 		break;
 	case ICE_TX_BUF_XDP_XMIT:
 		xdp_return_frame_bulk(tx_buf->xdpf, bq);
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 149911e3002a..eef16a909f85 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -302,7 +302,7 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector *q_vector,
 
 		/* free the skb */
 		if (ring_is_xdp(tx_ring))
-			page_frag_free(tx_buffer->data);
+			page_frag_free_va(tx_buffer->data);
 		else
 			napi_consume_skb(tx_buffer->skb, napi_budget);
 
@@ -2412,7 +2412,7 @@ static void ixgbevf_clean_tx_ring(struct ixgbevf_ring *tx_ring)
 
 		/* Free all the Tx ring sk_buffs */
 		if (ring_is_xdp(tx_ring))
-			page_frag_free(tx_buffer->data);
+			page_frag_free_va(tx_buffer->data);
 		else
 			dev_kfree_skb_any(tx_buffer->skb);
 
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index 87d5776e3b88..a485e988fa1d 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -553,7 +553,7 @@ static int __otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool,
 	*dma = dma_map_single_attrs(pfvf->dev, buf, pool->rbsize,
 				    DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
 	if (unlikely(dma_mapping_error(pfvf->dev, *dma))) {
-		page_frag_free(buf);
+		page_frag_free_va(buf);
 		return -ENOMEM;
 	}
 
diff --git a/drivers/net/ethernet/mediatek/mtk_wed_wo.c b/drivers/net/ethernet/mediatek/mtk_wed_wo.c
index 7063c78bd35f..c4228719f8a4 100644
--- a/drivers/net/ethernet/mediatek/mtk_wed_wo.c
+++ b/drivers/net/ethernet/mediatek/mtk_wed_wo.c
@@ -142,8 +142,8 @@ mtk_wed_wo_queue_refill(struct mtk_wed_wo *wo, struct mtk_wed_wo_queue *q,
 		dma_addr_t addr;
 		void *buf;
 
-		buf = page_frag_alloc(&q->cache, q->buf_size,
-				      GFP_ATOMIC | GFP_DMA32);
+		buf = page_frag_alloc_va(&q->cache, q->buf_size,
+					 GFP_ATOMIC | GFP_DMA32);
 		if (!buf)
 			break;
 
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index a2a47d3ab99f..86906bc505de 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -506,7 +506,7 @@ static void nvme_tcp_exit_request(struct blk_mq_tag_set *set,
 {
 	struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
 
-	page_frag_free(req->pdu);
+	page_frag_free_va(req->pdu);
 }
 
 static int nvme_tcp_init_request(struct blk_mq_tag_set *set,
@@ -520,7 +520,7 @@ static int nvme_tcp_init_request(struct blk_mq_tag_set *set,
 	struct nvme_tcp_queue *queue = &ctrl->queues[queue_idx];
 	u8 hdgst = nvme_tcp_hdgst_len(queue);
 
-	req->pdu = page_frag_alloc(&queue->pf_cache,
+	req->pdu = page_frag_alloc_va(&queue->pf_cache,
 		sizeof(struct nvme_tcp_cmd_pdu) + hdgst,
 		GFP_KERNEL | __GFP_ZERO);
 	if (!req->pdu)
@@ -1337,7 +1337,7 @@ static void nvme_tcp_free_async_req(struct nvme_tcp_ctrl *ctrl)
 {
 	struct nvme_tcp_request *async = &ctrl->async_req;
 
-	page_frag_free(async->pdu);
+	page_frag_free_va(async->pdu);
 }
 
 static int nvme_tcp_alloc_async_req(struct nvme_tcp_ctrl *ctrl)
@@ -1346,7 +1346,7 @@ static int nvme_tcp_alloc_async_req(struct nvme_tcp_ctrl *ctrl)
 	struct nvme_tcp_request *async = &ctrl->async_req;
 	u8 hdgst = nvme_tcp_hdgst_len(queue);
 
-	async->pdu = page_frag_alloc(&queue->pf_cache,
+	async->pdu = page_frag_alloc_va(&queue->pf_cache,
 		sizeof(struct nvme_tcp_cmd_pdu) + hdgst,
 		GFP_KERNEL | __GFP_ZERO);
 	if (!async->pdu)
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 5bff0d5464d1..560df3db2f82 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1463,24 +1463,24 @@ static int nvmet_tcp_alloc_cmd(struct nvmet_tcp_queue *queue,
 	c->queue = queue;
 	c->req.port = queue->port->nport;
 
-	c->cmd_pdu = page_frag_alloc(&queue->pf_cache,
+	c->cmd_pdu = page_frag_alloc_va(&queue->pf_cache,
 			sizeof(*c->cmd_pdu) + hdgst, GFP_KERNEL | __GFP_ZERO);
 	if (!c->cmd_pdu)
 		return -ENOMEM;
 	c->req.cmd = &c->cmd_pdu->cmd;
 
-	c->rsp_pdu = page_frag_alloc(&queue->pf_cache,
+	c->rsp_pdu = page_frag_alloc_va(&queue->pf_cache,
 			sizeof(*c->rsp_pdu) + hdgst, GFP_KERNEL | __GFP_ZERO);
 	if (!c->rsp_pdu)
 		goto out_free_cmd;
 	c->req.cqe = &c->rsp_pdu->cqe;
 
-	c->data_pdu = page_frag_alloc(&queue->pf_cache,
+	c->data_pdu = page_frag_alloc_va(&queue->pf_cache,
 			sizeof(*c->data_pdu) + hdgst, GFP_KERNEL | __GFP_ZERO);
 	if (!c->data_pdu)
 		goto out_free_rsp;
 
-	c->r2t_pdu = page_frag_alloc(&queue->pf_cache,
+	c->r2t_pdu = page_frag_alloc_va(&queue->pf_cache,
 			sizeof(*c->r2t_pdu) + hdgst, GFP_KERNEL | __GFP_ZERO);
 	if (!c->r2t_pdu)
 		goto out_free_data;
@@ -1495,20 +1495,20 @@ static int nvmet_tcp_alloc_cmd(struct nvmet_tcp_queue *queue,
 
 	return 0;
 out_free_data:
-	page_frag_free(c->data_pdu);
+	page_frag_free_va(c->data_pdu);
 out_free_rsp:
-	page_frag_free(c->rsp_pdu);
+	page_frag_free_va(c->rsp_pdu);
 out_free_cmd:
-	page_frag_free(c->cmd_pdu);
+	page_frag_free_va(c->cmd_pdu);
 	return -ENOMEM;
 }
 
 static void nvmet_tcp_free_cmd(struct nvmet_tcp_cmd *c)
 {
-	page_frag_free(c->r2t_pdu);
-	page_frag_free(c->data_pdu);
-	page_frag_free(c->rsp_pdu);
-	page_frag_free(c->cmd_pdu);
+	page_frag_free_va(c->r2t_pdu);
+	page_frag_free_va(c->data_pdu);
+	page_frag_free_va(c->rsp_pdu);
+	page_frag_free_va(c->cmd_pdu);
 }
 
 static int nvmet_tcp_alloc_cmds(struct nvmet_tcp_queue *queue)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index f16279351db5..6691fac01e0d 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -686,8 +686,8 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
 		return -ENOSPC;
 
 	buflen += SKB_DATA_ALIGN(len + pad);
-	buf = page_frag_alloc_align(&net->pf_cache, buflen, GFP_KERNEL,
-				    SMP_CACHE_BYTES);
+	buf = page_frag_alloc_va_align(&net->pf_cache, buflen, GFP_KERNEL,
+				       SMP_CACHE_BYTES);
 	if (unlikely(!buf))
 		return -ENOMEM;
 
@@ -734,7 +734,7 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
 	return 0;
 
 err:
-	page_frag_free(buf);
+	page_frag_free_va(buf);
 	return ret;
 }
 
diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
index a758cb65a9b3..ef038a07925c 100644
--- a/include/linux/page_frag_cache.h
+++ b/include/linux/page_frag_cache.h
@@ -9,23 +9,24 @@
 
 void page_frag_cache_drain(struct page_frag_cache *nc);
 void __page_frag_cache_drain(struct page *page, unsigned int count);
-void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz,
-			      gfp_t gfp_mask, unsigned int align_mask);
+void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
+				 unsigned int fragsz, gfp_t gfp_mask,
+				 unsigned int align_mask);
 
-static inline void *page_frag_alloc_align(struct page_frag_cache *nc,
-					  unsigned int fragsz, gfp_t gfp_mask,
-					  unsigned int align)
+static inline void *page_frag_alloc_va_align(struct page_frag_cache *nc,
+					     unsigned int fragsz,
+					     gfp_t gfp_mask, unsigned int align)
 {
 	WARN_ON_ONCE(!is_power_of_2(align));
-	return __page_frag_alloc_align(nc, fragsz, gfp_mask, -align);
+	return __page_frag_alloc_va_align(nc, fragsz, gfp_mask, -align);
 }
 
-static inline void *page_frag_alloc(struct page_frag_cache *nc,
-				    unsigned int fragsz, gfp_t gfp_mask)
+static inline void *page_frag_alloc_va(struct page_frag_cache *nc,
+				       unsigned int fragsz, gfp_t gfp_mask)
 {
-	return __page_frag_alloc_align(nc, fragsz, gfp_mask, ~0u);
+	return __page_frag_alloc_va_align(nc, fragsz, gfp_mask, ~0u);
 }
 
-void page_frag_free(void *addr);
+void page_frag_free_va(void *addr);
 
 #endif
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index e057db1c63e9..8d50cb3b161e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3381,7 +3381,7 @@ static inline struct sk_buff *netdev_alloc_skb_ip_align(struct net_device *dev,
 
 static inline void skb_free_frag(void *addr)
 {
-	page_frag_free(addr);
+	page_frag_free_va(addr);
 }
 
 void *__napi_alloc_frag_align(unsigned int fragsz, unsigned int align_mask);
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index fbdf5a1aabfe..3b70b6b071b9 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -323,7 +323,7 @@ static int cpu_map_kthread_run(void *data)
 
 			/* Bring struct page memory area to curr CPU. Read by
 			 * build_skb_around via page_is_pfmemalloc(), and when
-			 * freed written by page_frag_free call.
+			 * freed written by page_frag_free_va call.
 			 */
 			prefetchw(page);
 		}
diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
index c5bc72cf018a..70fb6dead624 100644
--- a/mm/page_frag_cache.c
+++ b/mm/page_frag_cache.c
@@ -59,9 +59,9 @@ void __page_frag_cache_drain(struct page *page, unsigned int count)
 }
 EXPORT_SYMBOL(__page_frag_cache_drain);
 
-void *__page_frag_alloc_align(struct page_frag_cache *nc,
-			      unsigned int fragsz, gfp_t gfp_mask,
-			      unsigned int align_mask)
+void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
+				 unsigned int fragsz, gfp_t gfp_mask,
+				 unsigned int align_mask)
 {
 #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
 	unsigned int size = nc->size;
@@ -130,16 +130,16 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
 
 	return nc->va + (size - remaining);
 }
-EXPORT_SYMBOL(__page_frag_alloc_align);
+EXPORT_SYMBOL(__page_frag_alloc_va_align);
 
 /*
  * Frees a page fragment allocated out of either a compound or order 0 page.
  */
-void page_frag_free(void *addr)
+void page_frag_free_va(void *addr)
 {
 	struct page *page = virt_to_head_page(addr);
 
 	if (unlikely(put_page_testzero(page)))
 		free_unref_page(page, compound_order(page));
 }
-EXPORT_SYMBOL(page_frag_free);
+EXPORT_SYMBOL(page_frag_free_va);
diff --git a/mm/page_frag_test.c b/mm/page_frag_test.c
index b7a5affb92f2..9eaa3ab74b29 100644
--- a/mm/page_frag_test.c
+++ b/mm/page_frag_test.c
@@ -276,7 +276,7 @@ static int page_frag_pop_thread(void *arg)
 
 		if (obj) {
 			nr--;
-			page_frag_free(obj);
+			page_frag_free_va(obj);
 		} else {
 			cond_resched();
 		}
@@ -304,13 +304,16 @@ static int page_frag_push_thread(void *arg)
 		int ret;
 
 		if (test_align) {
-			va = page_frag_alloc_align(&test_frag, test_alloc_len,
-						   GFP_KERNEL, SMP_CACHE_BYTES);
+			va = page_frag_alloc_va_align(&test_frag,
+						      test_alloc_len,
+						      GFP_KERNEL,
+						      SMP_CACHE_BYTES);
 
 			WARN_ONCE((unsigned long)va & (SMP_CACHE_BYTES - 1),
 				  "unaligned va returned\n");
 		} else {
-			va = page_frag_alloc(&test_frag, test_alloc_len, GFP_KERNEL);
+			va = page_frag_alloc_va(&test_frag, test_alloc_len,
+						GFP_KERNEL);
 		}
 
 		if (!va)
@@ -318,7 +321,7 @@ static int page_frag_push_thread(void *arg)
 
 		ret = objpool_push(va, pool);
 		if (ret) {
-			page_frag_free(va);
+			page_frag_free_va(va);
 			cond_resched();
 		} else {
 			nr--;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 83f8cd8aa2d1..4b8acd967793 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -314,8 +314,8 @@ void *__napi_alloc_frag_align(unsigned int fragsz, unsigned int align_mask)
 	fragsz = SKB_DATA_ALIGN(fragsz);
 
 	local_lock_nested_bh(&napi_alloc_cache.bh_lock);
-	data = __page_frag_alloc_align(&nc->page, fragsz, GFP_ATOMIC,
-				       align_mask);
+	data = __page_frag_alloc_va_align(&nc->page, fragsz, GFP_ATOMIC,
+					  align_mask);
 	local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
 	return data;
 
@@ -330,8 +330,8 @@ void *__netdev_alloc_frag_align(unsigned int fragsz, unsigned int align_mask)
 		struct page_frag_cache *nc = this_cpu_ptr(&netdev_alloc_cache);
 
 		fragsz = SKB_DATA_ALIGN(fragsz);
-		data = __page_frag_alloc_align(nc, fragsz, GFP_ATOMIC,
-					       align_mask);
+		data = __page_frag_alloc_va_align(nc, fragsz, GFP_ATOMIC,
+						  align_mask);
 	} else {
 		local_bh_disable();
 		data = __napi_alloc_frag_align(fragsz, align_mask);
@@ -748,14 +748,14 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev, unsigned int len,
 
 	if (in_hardirq() || irqs_disabled()) {
 		nc = this_cpu_ptr(&netdev_alloc_cache);
-		data = page_frag_alloc(nc, len, gfp_mask);
+		data = page_frag_alloc_va(nc, len, gfp_mask);
 		pfmemalloc = nc->pfmemalloc;
 	} else {
 		local_bh_disable();
 		local_lock_nested_bh(&napi_alloc_cache.bh_lock);
 
 		nc = this_cpu_ptr(&napi_alloc_cache.page);
-		data = page_frag_alloc(nc, len, gfp_mask);
+		data = page_frag_alloc_va(nc, len, gfp_mask);
 		pfmemalloc = nc->pfmemalloc;
 
 		local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
@@ -845,7 +845,7 @@ struct sk_buff *napi_alloc_skb(struct napi_struct *napi, unsigned int len)
 	} else {
 		len = SKB_HEAD_ALIGN(len);
 
-		data = page_frag_alloc(&nc->page, len, gfp_mask);
+		data = page_frag_alloc_va(&nc->page, len, gfp_mask);
 		pfmemalloc = nc->page.pfmemalloc;
 	}
 	local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
diff --git a/net/core/xdp.c b/net/core/xdp.c
index bcc5551c6424..7d4e09fb478f 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -387,7 +387,7 @@ void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
 		page_pool_put_full_page(page->pp, page, napi_direct);
 		break;
 	case MEM_TYPE_PAGE_SHARED:
-		page_frag_free(data);
+		page_frag_free_va(data);
 		break;
 	case MEM_TYPE_PAGE_ORDER0:
 		page = virt_to_page(data); /* Assumes order0 page*/
diff --git a/net/rxrpc/txbuf.c b/net/rxrpc/txbuf.c
index c3913d8a50d3..dccb0353ee84 100644
--- a/net/rxrpc/txbuf.c
+++ b/net/rxrpc/txbuf.c
@@ -33,8 +33,8 @@ struct rxrpc_txbuf *rxrpc_alloc_data_txbuf(struct rxrpc_call *call, size_t data_
 
 	data_align = umax(data_align, L1_CACHE_BYTES);
 	mutex_lock(&call->conn->tx_data_alloc_lock);
-	buf = page_frag_alloc_align(&call->conn->tx_data_alloc, total, gfp,
-				    data_align);
+	buf = page_frag_alloc_va_align(&call->conn->tx_data_alloc, total, gfp,
+				       data_align);
 	mutex_unlock(&call->conn->tx_data_alloc_lock);
 	if (!buf) {
 		kfree(txb);
@@ -96,17 +96,18 @@ struct rxrpc_txbuf *rxrpc_alloc_ack_txbuf(struct rxrpc_call *call, size_t sack_s
 	if (!txb)
 		return NULL;
 
-	buf = page_frag_alloc(&call->local->tx_alloc,
-			      sizeof(*whdr) + sizeof(*ack) + 1 + 3 + sizeof(*trailer), gfp);
+	buf = page_frag_alloc_va(&call->local->tx_alloc,
+				 sizeof(*whdr) + sizeof(*ack) + 1 + 3 + sizeof(*trailer), gfp);
 	if (!buf) {
 		kfree(txb);
 		return NULL;
 	}
 
 	if (sack_size) {
-		buf2 = page_frag_alloc(&call->local->tx_alloc, sack_size, gfp);
+		buf2 = page_frag_alloc_va(&call->local->tx_alloc, sack_size,
+					  gfp);
 		if (!buf2) {
-			page_frag_free(buf);
+			page_frag_free_va(buf);
 			kfree(txb);
 			return NULL;
 		}
@@ -180,7 +181,7 @@ static void rxrpc_free_txbuf(struct rxrpc_txbuf *txb)
 			  rxrpc_txbuf_free);
 	for (i = 0; i < txb->nr_kvec; i++)
 		if (txb->kvec[i].iov_base)
-			page_frag_free(txb->kvec[i].iov_base);
+			page_frag_free_va(txb->kvec[i].iov_base);
 	kfree(txb);
 	atomic_dec(&rxrpc_nr_txbuf);
 }
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 6b3f01beb294..42d20412c1c3 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1222,8 +1222,8 @@ static int svc_tcp_sendmsg(struct svc_sock *svsk, struct svc_rqst *rqstp,
 	/* The stream record marker is copied into a temporary page
 	 * fragment buffer so that it can be included in rq_bvec.
 	 */
-	buf = page_frag_alloc(&svsk->sk_frag_cache, sizeof(marker),
-			      GFP_KERNEL);
+	buf = page_frag_alloc_va(&svsk->sk_frag_cache, sizeof(marker),
+				 GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 	memcpy(buf, &marker, sizeof(marker));
@@ -1235,7 +1235,7 @@ static int svc_tcp_sendmsg(struct svc_sock *svsk, struct svc_rqst *rqstp,
 	iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, rqstp->rq_bvec,
 		      1 + count, sizeof(marker) + rqstp->rq_res.len);
 	ret = sock_sendmsg(svsk->sk_sock, &msg);
-	page_frag_free(buf);
+	page_frag_free_va(buf);
 	if (ret < 0)
 		return ret;
 	*sentp += ret;
-- 
2.33.0

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Chuck Lever]

On Wed, Jul 31, 2024 at 08:44:54PM +0800, Yunsheng Lin wrote:
> Currently the page_frag API is returning 'virtual address'
> or 'va' when allocing and expecting 'virtual address' or
> 'va' as input when freeing.
> 
> As we are about to support new use cases that the caller
> need to deal with 'struct page' or need to deal with both
> 'va' and 'struct page'. In order to differentiate the API
> handling between 'va' and 'struct page', add '_va' suffix
> to the corresponding API mirroring the page_pool_alloc_va()
> API of the page_pool. So that callers expecting to deal with
> va, page or both va and page may call page_frag_alloc_va*,
> page_frag_alloc_pg*, or page_frag_alloc* API accordingly.
> 
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> Reviewed-by: Subbaraya Sundeep <sbhatta@marvell.com>

For the net/sunrpc/svcsock.c hunk:

Acked-by: Chuck Lever <chuck.lever@oracle.com>


> ---
>  drivers/net/ethernet/google/gve/gve_rx.c      |  4 ++--
>  drivers/net/ethernet/intel/ice/ice_txrx.c     |  2 +-
>  drivers/net/ethernet/intel/ice/ice_txrx.h     |  2 +-
>  drivers/net/ethernet/intel/ice/ice_txrx_lib.c |  2 +-
>  .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |  4 ++--
>  .../marvell/octeontx2/nic/otx2_common.c       |  2 +-
>  drivers/net/ethernet/mediatek/mtk_wed_wo.c    |  4 ++--
>  drivers/nvme/host/tcp.c                       |  8 +++----
>  drivers/nvme/target/tcp.c                     | 22 +++++++++----------
>  drivers/vhost/net.c                           |  6 ++---
>  include/linux/page_frag_cache.h               | 21 +++++++++---------
>  include/linux/skbuff.h                        |  2 +-
>  kernel/bpf/cpumap.c                           |  2 +-
>  mm/page_frag_cache.c                          | 12 +++++-----
>  mm/page_frag_test.c                           | 13 ++++++-----
>  net/core/skbuff.c                             | 14 ++++++------
>  net/core/xdp.c                                |  2 +-
>  net/rxrpc/txbuf.c                             | 15 +++++++------
>  net/sunrpc/svcsock.c                          |  6 ++---
>  19 files changed, 74 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_rx.c b/drivers/net/ethernet/google/gve/gve_rx.c
> index acb73d4d0de6..b6c10100e462 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx.c
> @@ -729,7 +729,7 @@ static int gve_xdp_redirect(struct net_device *dev, struct gve_rx_ring *rx,
>  
>  	total_len = headroom + SKB_DATA_ALIGN(len) +
>  		SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> -	frame = page_frag_alloc(&rx->page_cache, total_len, GFP_ATOMIC);
> +	frame = page_frag_alloc_va(&rx->page_cache, total_len, GFP_ATOMIC);
>  	if (!frame) {
>  		u64_stats_update_begin(&rx->statss);
>  		rx->xdp_alloc_fails++;
> @@ -742,7 +742,7 @@ static int gve_xdp_redirect(struct net_device *dev, struct gve_rx_ring *rx,
>  
>  	err = xdp_do_redirect(dev, &new, xdp_prog);
>  	if (err)
> -		page_frag_free(frame);
> +		page_frag_free_va(frame);
>  
>  	return err;
>  }
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index 8bb743f78fcb..399b317c509d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -126,7 +126,7 @@ ice_unmap_and_free_tx_buf(struct ice_tx_ring *ring, struct ice_tx_buf *tx_buf)
>  		dev_kfree_skb_any(tx_buf->skb);
>  		break;
>  	case ICE_TX_BUF_XDP_TX:
> -		page_frag_free(tx_buf->raw_buf);
> +		page_frag_free_va(tx_buf->raw_buf);
>  		break;
>  	case ICE_TX_BUF_XDP_XMIT:
>  		xdp_return_frame(tx_buf->xdpf);
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
> index feba314a3fe4..6379f57d8228 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -148,7 +148,7 @@ static inline int ice_skb_pad(void)
>   * @ICE_TX_BUF_DUMMY: dummy Flow Director packet, unmap and kfree()
>   * @ICE_TX_BUF_FRAG: mapped skb OR &xdp_buff frag, only unmap DMA
>   * @ICE_TX_BUF_SKB: &sk_buff, unmap and consume_skb(), update stats
> - * @ICE_TX_BUF_XDP_TX: &xdp_buff, unmap and page_frag_free(), stats
> + * @ICE_TX_BUF_XDP_TX: &xdp_buff, unmap and page_frag_free_va(), stats
>   * @ICE_TX_BUF_XDP_XMIT: &xdp_frame, unmap and xdp_return_frame(), stats
>   * @ICE_TX_BUF_XSK_TX: &xdp_buff on XSk queue, xsk_buff_free(), stats
>   */
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> index 2719f0e20933..a1a41a14df0d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> @@ -250,7 +250,7 @@ ice_clean_xdp_tx_buf(struct device *dev, struct ice_tx_buf *tx_buf,
>  
>  	switch (tx_buf->type) {
>  	case ICE_TX_BUF_XDP_TX:
> -		page_frag_free(tx_buf->raw_buf);
> +		page_frag_free_va(tx_buf->raw_buf);
>  		break;
>  	case ICE_TX_BUF_XDP_XMIT:
>  		xdp_return_frame_bulk(tx_buf->xdpf, bq);
> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> index 149911e3002a..eef16a909f85 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> @@ -302,7 +302,7 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector *q_vector,
>  
>  		/* free the skb */
>  		if (ring_is_xdp(tx_ring))
> -			page_frag_free(tx_buffer->data);
> +			page_frag_free_va(tx_buffer->data);
>  		else
>  			napi_consume_skb(tx_buffer->skb, napi_budget);
>  
> @@ -2412,7 +2412,7 @@ static void ixgbevf_clean_tx_ring(struct ixgbevf_ring *tx_ring)
>  
>  		/* Free all the Tx ring sk_buffs */
>  		if (ring_is_xdp(tx_ring))
> -			page_frag_free(tx_buffer->data);
> +			page_frag_free_va(tx_buffer->data);
>  		else
>  			dev_kfree_skb_any(tx_buffer->skb);
>  
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 87d5776e3b88..a485e988fa1d 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -553,7 +553,7 @@ static int __otx2_alloc_rbuf(struct otx2_nic *pfvf, struct otx2_pool *pool,
>  	*dma = dma_map_single_attrs(pfvf->dev, buf, pool->rbsize,
>  				    DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
>  	if (unlikely(dma_mapping_error(pfvf->dev, *dma))) {
> -		page_frag_free(buf);
> +		page_frag_free_va(buf);
>  		return -ENOMEM;
>  	}
>  
> diff --git a/drivers/net/ethernet/mediatek/mtk_wed_wo.c b/drivers/net/ethernet/mediatek/mtk_wed_wo.c
> index 7063c78bd35f..c4228719f8a4 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed_wo.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed_wo.c
> @@ -142,8 +142,8 @@ mtk_wed_wo_queue_refill(struct mtk_wed_wo *wo, struct mtk_wed_wo_queue *q,
>  		dma_addr_t addr;
>  		void *buf;
>  
> -		buf = page_frag_alloc(&q->cache, q->buf_size,
> -				      GFP_ATOMIC | GFP_DMA32);
> +		buf = page_frag_alloc_va(&q->cache, q->buf_size,
> +					 GFP_ATOMIC | GFP_DMA32);
>  		if (!buf)
>  			break;
>  
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index a2a47d3ab99f..86906bc505de 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -506,7 +506,7 @@ static void nvme_tcp_exit_request(struct blk_mq_tag_set *set,
>  {
>  	struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
>  
> -	page_frag_free(req->pdu);
> +	page_frag_free_va(req->pdu);
>  }
>  
>  static int nvme_tcp_init_request(struct blk_mq_tag_set *set,
> @@ -520,7 +520,7 @@ static int nvme_tcp_init_request(struct blk_mq_tag_set *set,
>  	struct nvme_tcp_queue *queue = &ctrl->queues[queue_idx];
>  	u8 hdgst = nvme_tcp_hdgst_len(queue);
>  
> -	req->pdu = page_frag_alloc(&queue->pf_cache,
> +	req->pdu = page_frag_alloc_va(&queue->pf_cache,
>  		sizeof(struct nvme_tcp_cmd_pdu) + hdgst,
>  		GFP_KERNEL | __GFP_ZERO);
>  	if (!req->pdu)
> @@ -1337,7 +1337,7 @@ static void nvme_tcp_free_async_req(struct nvme_tcp_ctrl *ctrl)
>  {
>  	struct nvme_tcp_request *async = &ctrl->async_req;
>  
> -	page_frag_free(async->pdu);
> +	page_frag_free_va(async->pdu);
>  }
>  
>  static int nvme_tcp_alloc_async_req(struct nvme_tcp_ctrl *ctrl)
> @@ -1346,7 +1346,7 @@ static int nvme_tcp_alloc_async_req(struct nvme_tcp_ctrl *ctrl)
>  	struct nvme_tcp_request *async = &ctrl->async_req;
>  	u8 hdgst = nvme_tcp_hdgst_len(queue);
>  
> -	async->pdu = page_frag_alloc(&queue->pf_cache,
> +	async->pdu = page_frag_alloc_va(&queue->pf_cache,
>  		sizeof(struct nvme_tcp_cmd_pdu) + hdgst,
>  		GFP_KERNEL | __GFP_ZERO);
>  	if (!async->pdu)
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index 5bff0d5464d1..560df3db2f82 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -1463,24 +1463,24 @@ static int nvmet_tcp_alloc_cmd(struct nvmet_tcp_queue *queue,
>  	c->queue = queue;
>  	c->req.port = queue->port->nport;
>  
> -	c->cmd_pdu = page_frag_alloc(&queue->pf_cache,
> +	c->cmd_pdu = page_frag_alloc_va(&queue->pf_cache,
>  			sizeof(*c->cmd_pdu) + hdgst, GFP_KERNEL | __GFP_ZERO);
>  	if (!c->cmd_pdu)
>  		return -ENOMEM;
>  	c->req.cmd = &c->cmd_pdu->cmd;
>  
> -	c->rsp_pdu = page_frag_alloc(&queue->pf_cache,
> +	c->rsp_pdu = page_frag_alloc_va(&queue->pf_cache,
>  			sizeof(*c->rsp_pdu) + hdgst, GFP_KERNEL | __GFP_ZERO);
>  	if (!c->rsp_pdu)
>  		goto out_free_cmd;
>  	c->req.cqe = &c->rsp_pdu->cqe;
>  
> -	c->data_pdu = page_frag_alloc(&queue->pf_cache,
> +	c->data_pdu = page_frag_alloc_va(&queue->pf_cache,
>  			sizeof(*c->data_pdu) + hdgst, GFP_KERNEL | __GFP_ZERO);
>  	if (!c->data_pdu)
>  		goto out_free_rsp;
>  
> -	c->r2t_pdu = page_frag_alloc(&queue->pf_cache,
> +	c->r2t_pdu = page_frag_alloc_va(&queue->pf_cache,
>  			sizeof(*c->r2t_pdu) + hdgst, GFP_KERNEL | __GFP_ZERO);
>  	if (!c->r2t_pdu)
>  		goto out_free_data;
> @@ -1495,20 +1495,20 @@ static int nvmet_tcp_alloc_cmd(struct nvmet_tcp_queue *queue,
>  
>  	return 0;
>  out_free_data:
> -	page_frag_free(c->data_pdu);
> +	page_frag_free_va(c->data_pdu);
>  out_free_rsp:
> -	page_frag_free(c->rsp_pdu);
> +	page_frag_free_va(c->rsp_pdu);
>  out_free_cmd:
> -	page_frag_free(c->cmd_pdu);
> +	page_frag_free_va(c->cmd_pdu);
>  	return -ENOMEM;
>  }
>  
>  static void nvmet_tcp_free_cmd(struct nvmet_tcp_cmd *c)
>  {
> -	page_frag_free(c->r2t_pdu);
> -	page_frag_free(c->data_pdu);
> -	page_frag_free(c->rsp_pdu);
> -	page_frag_free(c->cmd_pdu);
> +	page_frag_free_va(c->r2t_pdu);
> +	page_frag_free_va(c->data_pdu);
> +	page_frag_free_va(c->rsp_pdu);
> +	page_frag_free_va(c->cmd_pdu);
>  }
>  
>  static int nvmet_tcp_alloc_cmds(struct nvmet_tcp_queue *queue)
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index f16279351db5..6691fac01e0d 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -686,8 +686,8 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
>  		return -ENOSPC;
>  
>  	buflen += SKB_DATA_ALIGN(len + pad);
> -	buf = page_frag_alloc_align(&net->pf_cache, buflen, GFP_KERNEL,
> -				    SMP_CACHE_BYTES);
> +	buf = page_frag_alloc_va_align(&net->pf_cache, buflen, GFP_KERNEL,
> +				       SMP_CACHE_BYTES);
>  	if (unlikely(!buf))
>  		return -ENOMEM;
>  
> @@ -734,7 +734,7 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
>  	return 0;
>  
>  err:
> -	page_frag_free(buf);
> +	page_frag_free_va(buf);
>  	return ret;
>  }
>  
> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
> index a758cb65a9b3..ef038a07925c 100644
> --- a/include/linux/page_frag_cache.h
> +++ b/include/linux/page_frag_cache.h
> @@ -9,23 +9,24 @@
>  
>  void page_frag_cache_drain(struct page_frag_cache *nc);
>  void __page_frag_cache_drain(struct page *page, unsigned int count);
> -void *__page_frag_alloc_align(struct page_frag_cache *nc, unsigned int fragsz,
> -			      gfp_t gfp_mask, unsigned int align_mask);
> +void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
> +				 unsigned int fragsz, gfp_t gfp_mask,
> +				 unsigned int align_mask);
>  
> -static inline void *page_frag_alloc_align(struct page_frag_cache *nc,
> -					  unsigned int fragsz, gfp_t gfp_mask,
> -					  unsigned int align)
> +static inline void *page_frag_alloc_va_align(struct page_frag_cache *nc,
> +					     unsigned int fragsz,
> +					     gfp_t gfp_mask, unsigned int align)
>  {
>  	WARN_ON_ONCE(!is_power_of_2(align));
> -	return __page_frag_alloc_align(nc, fragsz, gfp_mask, -align);
> +	return __page_frag_alloc_va_align(nc, fragsz, gfp_mask, -align);
>  }
>  
> -static inline void *page_frag_alloc(struct page_frag_cache *nc,
> -				    unsigned int fragsz, gfp_t gfp_mask)
> +static inline void *page_frag_alloc_va(struct page_frag_cache *nc,
> +				       unsigned int fragsz, gfp_t gfp_mask)
>  {
> -	return __page_frag_alloc_align(nc, fragsz, gfp_mask, ~0u);
> +	return __page_frag_alloc_va_align(nc, fragsz, gfp_mask, ~0u);
>  }
>  
> -void page_frag_free(void *addr);
> +void page_frag_free_va(void *addr);
>  
>  #endif
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index e057db1c63e9..8d50cb3b161e 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3381,7 +3381,7 @@ static inline struct sk_buff *netdev_alloc_skb_ip_align(struct net_device *dev,
>  
>  static inline void skb_free_frag(void *addr)
>  {
> -	page_frag_free(addr);
> +	page_frag_free_va(addr);
>  }
>  
>  void *__napi_alloc_frag_align(unsigned int fragsz, unsigned int align_mask);
> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> index fbdf5a1aabfe..3b70b6b071b9 100644
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c
> @@ -323,7 +323,7 @@ static int cpu_map_kthread_run(void *data)
>  
>  			/* Bring struct page memory area to curr CPU. Read by
>  			 * build_skb_around via page_is_pfmemalloc(), and when
> -			 * freed written by page_frag_free call.
> +			 * freed written by page_frag_free_va call.
>  			 */
>  			prefetchw(page);
>  		}
> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> index c5bc72cf018a..70fb6dead624 100644
> --- a/mm/page_frag_cache.c
> +++ b/mm/page_frag_cache.c
> @@ -59,9 +59,9 @@ void __page_frag_cache_drain(struct page *page, unsigned int count)
>  }
>  EXPORT_SYMBOL(__page_frag_cache_drain);
>  
> -void *__page_frag_alloc_align(struct page_frag_cache *nc,
> -			      unsigned int fragsz, gfp_t gfp_mask,
> -			      unsigned int align_mask)
> +void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
> +				 unsigned int fragsz, gfp_t gfp_mask,
> +				 unsigned int align_mask)
>  {
>  #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
>  	unsigned int size = nc->size;
> @@ -130,16 +130,16 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>  
>  	return nc->va + (size - remaining);
>  }
> -EXPORT_SYMBOL(__page_frag_alloc_align);
> +EXPORT_SYMBOL(__page_frag_alloc_va_align);
>  
>  /*
>   * Frees a page fragment allocated out of either a compound or order 0 page.
>   */
> -void page_frag_free(void *addr)
> +void page_frag_free_va(void *addr)
>  {
>  	struct page *page = virt_to_head_page(addr);
>  
>  	if (unlikely(put_page_testzero(page)))
>  		free_unref_page(page, compound_order(page));
>  }
> -EXPORT_SYMBOL(page_frag_free);
> +EXPORT_SYMBOL(page_frag_free_va);
> diff --git a/mm/page_frag_test.c b/mm/page_frag_test.c
> index b7a5affb92f2..9eaa3ab74b29 100644
> --- a/mm/page_frag_test.c
> +++ b/mm/page_frag_test.c
> @@ -276,7 +276,7 @@ static int page_frag_pop_thread(void *arg)
>  
>  		if (obj) {
>  			nr--;
> -			page_frag_free(obj);
> +			page_frag_free_va(obj);
>  		} else {
>  			cond_resched();
>  		}
> @@ -304,13 +304,16 @@ static int page_frag_push_thread(void *arg)
>  		int ret;
>  
>  		if (test_align) {
> -			va = page_frag_alloc_align(&test_frag, test_alloc_len,
> -						   GFP_KERNEL, SMP_CACHE_BYTES);
> +			va = page_frag_alloc_va_align(&test_frag,
> +						      test_alloc_len,
> +						      GFP_KERNEL,
> +						      SMP_CACHE_BYTES);
>  
>  			WARN_ONCE((unsigned long)va & (SMP_CACHE_BYTES - 1),
>  				  "unaligned va returned\n");
>  		} else {
> -			va = page_frag_alloc(&test_frag, test_alloc_len, GFP_KERNEL);
> +			va = page_frag_alloc_va(&test_frag, test_alloc_len,
> +						GFP_KERNEL);
>  		}
>  
>  		if (!va)
> @@ -318,7 +321,7 @@ static int page_frag_push_thread(void *arg)
>  
>  		ret = objpool_push(va, pool);
>  		if (ret) {
> -			page_frag_free(va);
> +			page_frag_free_va(va);
>  			cond_resched();
>  		} else {
>  			nr--;
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 83f8cd8aa2d1..4b8acd967793 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -314,8 +314,8 @@ void *__napi_alloc_frag_align(unsigned int fragsz, unsigned int align_mask)
>  	fragsz = SKB_DATA_ALIGN(fragsz);
>  
>  	local_lock_nested_bh(&napi_alloc_cache.bh_lock);
> -	data = __page_frag_alloc_align(&nc->page, fragsz, GFP_ATOMIC,
> -				       align_mask);
> +	data = __page_frag_alloc_va_align(&nc->page, fragsz, GFP_ATOMIC,
> +					  align_mask);
>  	local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
>  	return data;
>  
> @@ -330,8 +330,8 @@ void *__netdev_alloc_frag_align(unsigned int fragsz, unsigned int align_mask)
>  		struct page_frag_cache *nc = this_cpu_ptr(&netdev_alloc_cache);
>  
>  		fragsz = SKB_DATA_ALIGN(fragsz);
> -		data = __page_frag_alloc_align(nc, fragsz, GFP_ATOMIC,
> -					       align_mask);
> +		data = __page_frag_alloc_va_align(nc, fragsz, GFP_ATOMIC,
> +						  align_mask);
>  	} else {
>  		local_bh_disable();
>  		data = __napi_alloc_frag_align(fragsz, align_mask);
> @@ -748,14 +748,14 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev, unsigned int len,
>  
>  	if (in_hardirq() || irqs_disabled()) {
>  		nc = this_cpu_ptr(&netdev_alloc_cache);
> -		data = page_frag_alloc(nc, len, gfp_mask);
> +		data = page_frag_alloc_va(nc, len, gfp_mask);
>  		pfmemalloc = nc->pfmemalloc;
>  	} else {
>  		local_bh_disable();
>  		local_lock_nested_bh(&napi_alloc_cache.bh_lock);
>  
>  		nc = this_cpu_ptr(&napi_alloc_cache.page);
> -		data = page_frag_alloc(nc, len, gfp_mask);
> +		data = page_frag_alloc_va(nc, len, gfp_mask);
>  		pfmemalloc = nc->pfmemalloc;
>  
>  		local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
> @@ -845,7 +845,7 @@ struct sk_buff *napi_alloc_skb(struct napi_struct *napi, unsigned int len)
>  	} else {
>  		len = SKB_HEAD_ALIGN(len);
>  
> -		data = page_frag_alloc(&nc->page, len, gfp_mask);
> +		data = page_frag_alloc_va(&nc->page, len, gfp_mask);
>  		pfmemalloc = nc->page.pfmemalloc;
>  	}
>  	local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index bcc5551c6424..7d4e09fb478f 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -387,7 +387,7 @@ void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
>  		page_pool_put_full_page(page->pp, page, napi_direct);
>  		break;
>  	case MEM_TYPE_PAGE_SHARED:
> -		page_frag_free(data);
> +		page_frag_free_va(data);
>  		break;
>  	case MEM_TYPE_PAGE_ORDER0:
>  		page = virt_to_page(data); /* Assumes order0 page*/
> diff --git a/net/rxrpc/txbuf.c b/net/rxrpc/txbuf.c
> index c3913d8a50d3..dccb0353ee84 100644
> --- a/net/rxrpc/txbuf.c
> +++ b/net/rxrpc/txbuf.c
> @@ -33,8 +33,8 @@ struct rxrpc_txbuf *rxrpc_alloc_data_txbuf(struct rxrpc_call *call, size_t data_
>  
>  	data_align = umax(data_align, L1_CACHE_BYTES);
>  	mutex_lock(&call->conn->tx_data_alloc_lock);
> -	buf = page_frag_alloc_align(&call->conn->tx_data_alloc, total, gfp,
> -				    data_align);
> +	buf = page_frag_alloc_va_align(&call->conn->tx_data_alloc, total, gfp,
> +				       data_align);
>  	mutex_unlock(&call->conn->tx_data_alloc_lock);
>  	if (!buf) {
>  		kfree(txb);
> @@ -96,17 +96,18 @@ struct rxrpc_txbuf *rxrpc_alloc_ack_txbuf(struct rxrpc_call *call, size_t sack_s
>  	if (!txb)
>  		return NULL;
>  
> -	buf = page_frag_alloc(&call->local->tx_alloc,
> -			      sizeof(*whdr) + sizeof(*ack) + 1 + 3 + sizeof(*trailer), gfp);
> +	buf = page_frag_alloc_va(&call->local->tx_alloc,
> +				 sizeof(*whdr) + sizeof(*ack) + 1 + 3 + sizeof(*trailer), gfp);
>  	if (!buf) {
>  		kfree(txb);
>  		return NULL;
>  	}
>  
>  	if (sack_size) {
> -		buf2 = page_frag_alloc(&call->local->tx_alloc, sack_size, gfp);
> +		buf2 = page_frag_alloc_va(&call->local->tx_alloc, sack_size,
> +					  gfp);
>  		if (!buf2) {
> -			page_frag_free(buf);
> +			page_frag_free_va(buf);
>  			kfree(txb);
>  			return NULL;
>  		}
> @@ -180,7 +181,7 @@ static void rxrpc_free_txbuf(struct rxrpc_txbuf *txb)
>  			  rxrpc_txbuf_free);
>  	for (i = 0; i < txb->nr_kvec; i++)
>  		if (txb->kvec[i].iov_base)
> -			page_frag_free(txb->kvec[i].iov_base);
> +			page_frag_free_va(txb->kvec[i].iov_base);
>  	kfree(txb);
>  	atomic_dec(&rxrpc_nr_txbuf);
>  }
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 6b3f01beb294..42d20412c1c3 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -1222,8 +1222,8 @@ static int svc_tcp_sendmsg(struct svc_sock *svsk, struct svc_rqst *rqstp,
>  	/* The stream record marker is copied into a temporary page
>  	 * fragment buffer so that it can be included in rq_bvec.
>  	 */
> -	buf = page_frag_alloc(&svsk->sk_frag_cache, sizeof(marker),
> -			      GFP_KERNEL);
> +	buf = page_frag_alloc_va(&svsk->sk_frag_cache, sizeof(marker),
> +				 GFP_KERNEL);
>  	if (!buf)
>  		return -ENOMEM;
>  	memcpy(buf, &marker, sizeof(marker));
> @@ -1235,7 +1235,7 @@ static int svc_tcp_sendmsg(struct svc_sock *svsk, struct svc_rqst *rqstp,
>  	iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, rqstp->rq_bvec,
>  		      1 + count, sizeof(marker) + rqstp->rq_res.len);
>  	ret = sock_sendmsg(svsk->sk_sock, &msg);
> -	page_frag_free(buf);
> +	page_frag_free_va(buf);
>  	if (ret < 0)
>  		return ret;
>  	*sentp += ret;
> -- 
> 2.33.0
> 

-- 
Chuck Lever

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Alexander Duyck]

On Wed, Jul 31, 2024 at 5:50 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> Currently the page_frag API is returning 'virtual address'
> or 'va' when allocing and expecting 'virtual address' or
> 'va' as input when freeing.
>
> As we are about to support new use cases that the caller
> need to deal with 'struct page' or need to deal with both
> 'va' and 'struct page'. In order to differentiate the API
> handling between 'va' and 'struct page', add '_va' suffix
> to the corresponding API mirroring the page_pool_alloc_va()
> API of the page_pool. So that callers expecting to deal with
> va, page or both va and page may call page_frag_alloc_va*,
> page_frag_alloc_pg*, or page_frag_alloc* API accordingly.
>
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> Reviewed-by: Subbaraya Sundeep <sbhatta@marvell.com>

I am naking this patch. It is a pointless rename that is just going to
obfuscate the git history for these callers.

As I believe I said before I would prefer to see this work more like
the handling of __get_free_pages and __free_pages in terms of the use
of pages versus pointers and/or longs. Pushing this API aside because
you want to reuse the name for something different isn't a valid
reason to rename an existing API and will just lead to confusion.

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Yunsheng Lin]

On 2024/8/1 2:13, Alexander Duyck wrote:
> On Wed, Jul 31, 2024 at 5:50 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> Currently the page_frag API is returning 'virtual address'
>> or 'va' when allocing and expecting 'virtual address' or
>> 'va' as input when freeing.
>>
>> As we are about to support new use cases that the caller
>> need to deal with 'struct page' or need to deal with both
>> 'va' and 'struct page'. In order to differentiate the API
>> handling between 'va' and 'struct page', add '_va' suffix
>> to the corresponding API mirroring the page_pool_alloc_va()
>> API of the page_pool. So that callers expecting to deal with
>> va, page or both va and page may call page_frag_alloc_va*,
>> page_frag_alloc_pg*, or page_frag_alloc* API accordingly.
>>
>> CC: Alexander Duyck <alexander.duyck@gmail.com>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> Reviewed-by: Subbaraya Sundeep <sbhatta@marvell.com>
> 
> I am naking this patch. It is a pointless rename that is just going to
> obfuscate the git history for these callers.

I responded to your above similar comment in v2, and then responded more
detailedly in v11, both got not direct responding, it would be good to
have more concrete feedback here instead of abstract argument.

https://lore.kernel.org/all/74e7259a-c462-e3c1-73ac-8e3f49fb80b8@huawei.com/
https://lore.kernel.org/all/11187fe4-9419-4341-97b5-6dad7583b5b6@huawei.com/

> 
> As I believe I said before I would prefer to see this work more like
> the handling of __get_free_pages and __free_pages in terms of the use

I am not even sure why are you bringing up  __get_free_pages() and
__free_pages() here, as the declaration of them is something like below:

unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order);
void __free_pages(struct page *page, unsigned int order);

And I add another related one for completeness here:
extern void free_pages(unsigned long addr, unsigned int order);

I am failing to see there is any pattern or rule for the above API
naming. If there is some pattern for the above existing APIs, please
describe them in detail so that we have common understanding.

After the renaming, the declaration for both new and old APIs is
below, please be more specific about what exactly is the confusion
about them, what is the better naming for the below APIs in your
mind:
struct page *page_frag_alloc_pg(struct page_frag_cache *nc,
                               unsigned int *offset, unsigned int fragsz,
                               gfp_t gfp);
void *page_frag_alloc_va(struct page_frag_cache *nc,
                         unsigned int fragsz, gfp_t gfp_mask);
struct page *page_frag_alloc(struct page_frag_cache *nc,
                             unsigned int *offset,
                             unsigned int fragsz,
                             void **va, gfp_t gfp);

> of pages versus pointers and/or longs. Pushing this API aside because
> you want to reuse the name for something different isn't a valid
> reason to rename an existing API and will just lead to confusion.

Before this patchset, all the page_frag API renamed with a '_va' suffix
in this patch are dealing with virtual address, it would be better to be
more specific about what exactly is the confusion here by adding a explicit
'va' suffix for them in this patch?

I would argue that the renaming may avoid some confusion about whether
page_frag_alloc() returning a 'struct page' or returning a virtual address
instead of leading to confusion.

Anyway, naming is always hard, any better naming is welcome. But don't deny
any existing API renaming when we have not really started doing detailed
comparison between different API naming options yet.

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Alexander Duyck]

On Thu, Aug 1, 2024 at 6:01 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2024/8/1 2:13, Alexander Duyck wrote:
> > On Wed, Jul 31, 2024 at 5:50 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >>
> >> Currently the page_frag API is returning 'virtual address'
> >> or 'va' when allocing and expecting 'virtual address' or
> >> 'va' as input when freeing.
> >>
> >> As we are about to support new use cases that the caller
> >> need to deal with 'struct page' or need to deal with both
> >> 'va' and 'struct page'. In order to differentiate the API
> >> handling between 'va' and 'struct page', add '_va' suffix
> >> to the corresponding API mirroring the page_pool_alloc_va()
> >> API of the page_pool. So that callers expecting to deal with
> >> va, page or both va and page may call page_frag_alloc_va*,
> >> page_frag_alloc_pg*, or page_frag_alloc* API accordingly.
> >>
> >> CC: Alexander Duyck <alexander.duyck@gmail.com>
> >> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> >> Reviewed-by: Subbaraya Sundeep <sbhatta@marvell.com>
> >
> > I am naking this patch. It is a pointless rename that is just going to
> > obfuscate the git history for these callers.
>
> I responded to your above similar comment in v2, and then responded more
> detailedly in v11, both got not direct responding, it would be good to
> have more concrete feedback here instead of abstract argument.
>
> https://lore.kernel.org/all/74e7259a-c462-e3c1-73ac-8e3f49fb80b8@huawei.com/
> https://lore.kernel.org/all/11187fe4-9419-4341-97b5-6dad7583b5b6@huawei.com/

I will make this much more understandable. This patch is one of the
ones that will permanently block this set in my opinion. As such I
will never ack this patch as I see no benefit to it. Arguing with me
on this is moot as you aren't going to change my mind, and I don't
have all day to argue back and forth with you on every single patch.

As far as your API extension and naming maybe you should look like
something like bio_vec and borrow the naming from that since that is
essentially what you are passing back and forth is essentially that
instead of a page frag which is normally a virtual address.

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Yunsheng Lin]

On 2024/8/1 23:21, Alexander Duyck wrote:
> On Thu, Aug 1, 2024 at 6:01 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> On 2024/8/1 2:13, Alexander Duyck wrote:
>>> On Wed, Jul 31, 2024 at 5:50 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>>>
>>>> Currently the page_frag API is returning 'virtual address'
>>>> or 'va' when allocing and expecting 'virtual address' or
>>>> 'va' as input when freeing.
>>>>
>>>> As we are about to support new use cases that the caller
>>>> need to deal with 'struct page' or need to deal with both
>>>> 'va' and 'struct page'. In order to differentiate the API
>>>> handling between 'va' and 'struct page', add '_va' suffix
>>>> to the corresponding API mirroring the page_pool_alloc_va()
>>>> API of the page_pool. So that callers expecting to deal with
>>>> va, page or both va and page may call page_frag_alloc_va*,
>>>> page_frag_alloc_pg*, or page_frag_alloc* API accordingly.
>>>>
>>>> CC: Alexander Duyck <alexander.duyck@gmail.com>
>>>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>>>> Reviewed-by: Subbaraya Sundeep <sbhatta@marvell.com>
>>>
>>> I am naking this patch. It is a pointless rename that is just going to
>>> obfuscate the git history for these callers.
>>
>> I responded to your above similar comment in v2, and then responded more
>> detailedly in v11, both got not direct responding, it would be good to
>> have more concrete feedback here instead of abstract argument.
>>
>> https://lore.kernel.org/all/74e7259a-c462-e3c1-73ac-8e3f49fb80b8@huawei.com/
>> https://lore.kernel.org/all/11187fe4-9419-4341-97b5-6dad7583b5b6@huawei.com/
> 
> I will make this much more understandable. This patch is one of the
> ones that will permanently block this set in my opinion. As such I
> will never ack this patch as I see no benefit to it. Arguing with me
> on this is moot as you aren't going to change my mind, and I don't
> have all day to argue back and forth with you on every single patch.

Let's move on to more specific technical discussion then.

> 
> As far as your API extension and naming maybe you should look like
> something like bio_vec and borrow the naming from that since that is
> essentially what you are passing back and forth is essentially that
> instead of a page frag which is normally a virtual address.

I thought about adding something like bio_vec before, but I am not sure
what you have in mind is somthing like I considered before?
Let's say that we reuse bio_vec like something below for the new APIs:

struct bio_vec {
	struct page	*bv_page;
	void		*va;
	unsigned int	bv_len;
	unsigned int	bv_offset;
};

It seems we have the below options for the new API:

option 1, it seems like a better option from API naming point of view, but
it needs to return a bio_vec pointer to the caller, it seems we need to have
extra space for the pointer, I am not sure how we can avoid the memory waste
for sk_page_frag() case in patch 12:
struct bio_vec *page_frag_alloc_bio(struct page_frag_cache *nc,
				    unsigned int fragsz, gfp_t gfp_mask);

option 2, it need both the caller and callee to have a its own local space
for 'struct bio_vec ', I am not sure if passing the content instead of
the pointer of a struct through the function returning is the common pattern
and if it has any performance impact yet:
struct bio_vec page_frag_alloc_bio(struct page_frag_cache *nc,
				   unsigned int fragsz, gfp_t gfp_mask);

option 3, the caller passes the pointer of 'struct bio_vec ' to the callee,
and page_frag_alloc_bio() fills in the data, I am not sure what is the point
of indirect using 'struct bio_vec ' instead of passing 'va' & 'fragsz' &
'offset' through pointers directly:
bool page_frag_alloc_bio(struct page_frag_cache *nc,
			 unsigned int fragsz, gfp_t gfp_mask, struct bio_vec *bio);

If one of the above option is something in your mind? Yes, please be more specific
about which one is the prefer option, and why it is the prefer option than the one
introduced in this patchset?

If no, please be more specific what that is in your mind?

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Alexander Duyck]

On Fri, Aug 2, 2024 at 3:05 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2024/8/1 23:21, Alexander Duyck wrote:
> > On Thu, Aug 1, 2024 at 6:01 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >>
> >> On 2024/8/1 2:13, Alexander Duyck wrote:
> >>> On Wed, Jul 31, 2024 at 5:50 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >>>>
> >>>> Currently the page_frag API is returning 'virtual address'
> >>>> or 'va' when allocing and expecting 'virtual address' or
> >>>> 'va' as input when freeing.
> >>>>
> >>>> As we are about to support new use cases that the caller
> >>>> need to deal with 'struct page' or need to deal with both
> >>>> 'va' and 'struct page'. In order to differentiate the API
> >>>> handling between 'va' and 'struct page', add '_va' suffix
> >>>> to the corresponding API mirroring the page_pool_alloc_va()
> >>>> API of the page_pool. So that callers expecting to deal with
> >>>> va, page or both va and page may call page_frag_alloc_va*,
> >>>> page_frag_alloc_pg*, or page_frag_alloc* API accordingly.
> >>>>
> >>>> CC: Alexander Duyck <alexander.duyck@gmail.com>
> >>>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> >>>> Reviewed-by: Subbaraya Sundeep <sbhatta@marvell.com>
> >>>
> >>> I am naking this patch. It is a pointless rename that is just going to
> >>> obfuscate the git history for these callers.
> >>
> >> I responded to your above similar comment in v2, and then responded more
> >> detailedly in v11, both got not direct responding, it would be good to
> >> have more concrete feedback here instead of abstract argument.
> >>
> >> https://lore.kernel.org/all/74e7259a-c462-e3c1-73ac-8e3f49fb80b8@huawei.com/
> >> https://lore.kernel.org/all/11187fe4-9419-4341-97b5-6dad7583b5b6@huawei.com/
> >
> > I will make this much more understandable. This patch is one of the
> > ones that will permanently block this set in my opinion. As such I
> > will never ack this patch as I see no benefit to it. Arguing with me
> > on this is moot as you aren't going to change my mind, and I don't
> > have all day to argue back and forth with you on every single patch.
>
> Let's move on to more specific technical discussion then.
>
> >
> > As far as your API extension and naming maybe you should look like
> > something like bio_vec and borrow the naming from that since that is
> > essentially what you are passing back and forth is essentially that
> > instead of a page frag which is normally a virtual address.
>
> I thought about adding something like bio_vec before, but I am not sure
> what you have in mind is somthing like I considered before?
> Let's say that we reuse bio_vec like something below for the new APIs:
>
> struct bio_vec {
>         struct page     *bv_page;
>         void            *va;
>         unsigned int    bv_len;
>         unsigned int    bv_offset;
> };

I wasn't suggesting changing the bio_vec. I was suggesting that be
what you pass as a pointer reference instead of the offset. Basically
your use case is mostly just for populating bio_vec style structures
anyway.

> It seems we have the below options for the new API:
>
> option 1, it seems like a better option from API naming point of view, but
> it needs to return a bio_vec pointer to the caller, it seems we need to have
> extra space for the pointer, I am not sure how we can avoid the memory waste
> for sk_page_frag() case in patch 12:
> struct bio_vec *page_frag_alloc_bio(struct page_frag_cache *nc,
>                                     unsigned int fragsz, gfp_t gfp_mask);
>
> option 2, it need both the caller and callee to have a its own local space
> for 'struct bio_vec ', I am not sure if passing the content instead of
> the pointer of a struct through the function returning is the common pattern
> and if it has any performance impact yet:
> struct bio_vec page_frag_alloc_bio(struct page_frag_cache *nc,
>                                    unsigned int fragsz, gfp_t gfp_mask);
>
> option 3, the caller passes the pointer of 'struct bio_vec ' to the callee,
> and page_frag_alloc_bio() fills in the data, I am not sure what is the point
> of indirect using 'struct bio_vec ' instead of passing 'va' & 'fragsz' &
> 'offset' through pointers directly:
> bool page_frag_alloc_bio(struct page_frag_cache *nc,
>                          unsigned int fragsz, gfp_t gfp_mask, struct bio_vec *bio);
>
> If one of the above option is something in your mind? Yes, please be more specific
> about which one is the prefer option, and why it is the prefer option than the one
> introduced in this patchset?
>
> If no, please be more specific what that is in your mind?

Option 3 is more or less what I had in mind. Basically you would
return an int to indicate any errors and you would be populating a
bio_vec during your allocation. In addition you would use the bio_vec
as a tracker of the actual fragsz so when you commit you are
committing with the fragsz as it was determined at the time of putting
the bio_vec together so you can theoretically catch things like if the
underlying offset had somehow changed from the time you setup the
allocation. It would fit well into your probe routines since they are
all essentially passing the page, offset, and fragsz throughout the
code.

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Yunsheng Lin]

On 8/3/2024 1:00 AM, Alexander Duyck wrote:

>>
>>>
>>> As far as your API extension and naming maybe you should look like
>>> something like bio_vec and borrow the naming from that since that is
>>> essentially what you are passing back and forth is essentially that
>>> instead of a page frag which is normally a virtual address.
>>
>> I thought about adding something like bio_vec before, but I am not sure
>> what you have in mind is somthing like I considered before?
>> Let's say that we reuse bio_vec like something below for the new APIs:
>>
>> struct bio_vec {
>>          struct page     *bv_page;
>>          void            *va;
>>          unsigned int    bv_len;
>>          unsigned int    bv_offset;
>> };
> 
> I wasn't suggesting changing the bio_vec. I was suggesting that be
> what you pass as a pointer reference instead of the offset. Basically
> your use case is mostly just for populating bio_vec style structures
> anyway.

I wasn't trying/going to reuse/change bio_vec for page_frag, I was just
having a hard time coming with a good new name for it.
The best one I came up with is pfrag_vec, but I am not sure about the
'vec' as the "vec" portion of the name would suggest, iovec structures 
tend to come in arrays, mentioned in the below article:
https://lwn.net/Articles/625077/

Anther one is page_frag, which is currently in use.

Or any better one in your mind?

> 
>> It seems we have the below options for the new API:
>>
>> option 1, it seems like a better option from API naming point of view, but
>> it needs to return a bio_vec pointer to the caller, it seems we need to have
>> extra space for the pointer, I am not sure how we can avoid the memory waste
>> for sk_page_frag() case in patch 12:
>> struct bio_vec *page_frag_alloc_bio(struct page_frag_cache *nc,
>>                                      unsigned int fragsz, gfp_t gfp_mask);
>>
>> option 2, it need both the caller and callee to have a its own local space
>> for 'struct bio_vec ', I am not sure if passing the content instead of
>> the pointer of a struct through the function returning is the common pattern
>> and if it has any performance impact yet:
>> struct bio_vec page_frag_alloc_bio(struct page_frag_cache *nc,
>>                                     unsigned int fragsz, gfp_t gfp_mask);
>>
>> option 3, the caller passes the pointer of 'struct bio_vec ' to the callee,
>> and page_frag_alloc_bio() fills in the data, I am not sure what is the point
>> of indirect using 'struct bio_vec ' instead of passing 'va' & 'fragsz' &
>> 'offset' through pointers directly:
>> bool page_frag_alloc_bio(struct page_frag_cache *nc,
>>                           unsigned int fragsz, gfp_t gfp_mask, struct bio_vec *bio);
>>
>> If one of the above option is something in your mind? Yes, please be more specific
>> about which one is the prefer option, and why it is the prefer option than the one
>> introduced in this patchset?
>>
>> If no, please be more specific what that is in your mind?
> 
> Option 3 is more or less what I had in mind. Basically you would
> return an int to indicate any errors and you would be populating a
> bio_vec during your allocation. In addition you would use the bio_vec

Actually using this new bio_vec style structures does not seem to solve
the APIs naming issue this patch is trying to solve as my understanding,
as the new struct is only about passing one pointer or multi-pointers
from API naming perspective. It is part of the API naming, but not all
of it.

> as a tracker of the actual fragsz so when you commit you are
> committing with the fragsz as it was determined at the time of putting
> the bio_vec together so you can theoretically catch things like if the
> underlying offset had somehow changed from the time you setup the

I think we might need a stronger argument than the above to use the new
*vec thing other than the above debugging feature.

I looked throught the bio_vec related info, and come along somewhat not
really related, but really helpful "What’s all this get us" section:
https://docs.kernel.org/block/biovecs.html

So the question seems to be: what is this new struct for page_frag get
us?

Generally, I am argeed with the new struct thing if it does bring us
something other than the above debugging feature. Otherwise we should
avoid introducing a new thing which is hard to argue about its existent.

> allocation. It would fit well into your probe routines since they are
> all essentially passing the page, offset, and fragsz throughout the
> code.

For the current probe routines, the 'va' need to be passed, do you
expect the 'va' to be passed by function return, double pointer, or
new the *_vec pointer?

> 

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Alexander Duyck]

On Sun, Aug 4, 2024 at 10:00 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote:
>
> On 8/3/2024 1:00 AM, Alexander Duyck wrote:
>
> >>
> >>>
> >>> As far as your API extension and naming maybe you should look like
> >>> something like bio_vec and borrow the naming from that since that is
> >>> essentially what you are passing back and forth is essentially that
> >>> instead of a page frag which is normally a virtual address.
> >>
> >> I thought about adding something like bio_vec before, but I am not sure
> >> what you have in mind is somthing like I considered before?
> >> Let's say that we reuse bio_vec like something below for the new APIs:
> >>
> >> struct bio_vec {
> >>          struct page     *bv_page;
> >>          void            *va;
> >>          unsigned int    bv_len;
> >>          unsigned int    bv_offset;
> >> };
> >
> > I wasn't suggesting changing the bio_vec. I was suggesting that be
> > what you pass as a pointer reference instead of the offset. Basically
> > your use case is mostly just for populating bio_vec style structures
> > anyway.
>
> I wasn't trying/going to reuse/change bio_vec for page_frag, I was just
> having a hard time coming with a good new name for it.
> The best one I came up with is pfrag_vec, but I am not sure about the
> 'vec' as the "vec" portion of the name would suggest, iovec structures
> tend to come in arrays, mentioned in the below article:
> https://lwn.net/Articles/625077/
>
> Anther one is page_frag, which is currently in use.
>
> Or any better one in your mind?

I was suggesting using bio_vec, not some new structure. The general
idea is that almost all the values you are using are exposed by that
structure already in the case of the page based calls you were adding,
so it makes sense to use what is there rather than reinventing the
wheel.

> >
> >> It seems we have the below options for the new API:
> >>
> >> option 1, it seems like a better option from API naming point of view, but
> >> it needs to return a bio_vec pointer to the caller, it seems we need to have
> >> extra space for the pointer, I am not sure how we can avoid the memory waste
> >> for sk_page_frag() case in patch 12:
> >> struct bio_vec *page_frag_alloc_bio(struct page_frag_cache *nc,
> >>                                      unsigned int fragsz, gfp_t gfp_mask);
> >>
> >> option 2, it need both the caller and callee to have a its own local space
> >> for 'struct bio_vec ', I am not sure if passing the content instead of
> >> the pointer of a struct through the function returning is the common pattern
> >> and if it has any performance impact yet:
> >> struct bio_vec page_frag_alloc_bio(struct page_frag_cache *nc,
> >>                                     unsigned int fragsz, gfp_t gfp_mask);
> >>
> >> option 3, the caller passes the pointer of 'struct bio_vec ' to the callee,
> >> and page_frag_alloc_bio() fills in the data, I am not sure what is the point
> >> of indirect using 'struct bio_vec ' instead of passing 'va' & 'fragsz' &
> >> 'offset' through pointers directly:
> >> bool page_frag_alloc_bio(struct page_frag_cache *nc,
> >>                           unsigned int fragsz, gfp_t gfp_mask, struct bio_vec *bio);
> >>
> >> If one of the above option is something in your mind? Yes, please be more specific
> >> about which one is the prefer option, and why it is the prefer option than the one
> >> introduced in this patchset?
> >>
> >> If no, please be more specific what that is in your mind?
> >
> > Option 3 is more or less what I had in mind. Basically you would
> > return an int to indicate any errors and you would be populating a
> > bio_vec during your allocation. In addition you would use the bio_vec
>
> Actually using this new bio_vec style structures does not seem to solve
> the APIs naming issue this patch is trying to solve as my understanding,
> as the new struct is only about passing one pointer or multi-pointers
> from API naming perspective. It is part of the API naming, but not all
> of it.

I have no idea what you are talking about. The issue was you were
splitting things page_frag_alloc_va and page_frag_alloc_pg. Now it
would be page_frag_alloc and page_frag_alloc_bio or maybe
page_frag_fill_bio which would better explain what you are doing with
this function.

> > as a tracker of the actual fragsz so when you commit you are
> > committing with the fragsz as it was determined at the time of putting
> > the bio_vec together so you can theoretically catch things like if the
> > underlying offset had somehow changed from the time you setup the
>
> I think we might need a stronger argument than the above to use the new
> *vec thing other than the above debugging feature.
>
> I looked throught the bio_vec related info, and come along somewhat not
> really related, but really helpful "What’s all this get us" section:
> https://docs.kernel.org/block/biovecs.html
>
> So the question seems to be: what is this new struct for page_frag get
> us?
>
> Generally, I am argeed with the new struct thing if it does bring us
> something other than the above debugging feature. Otherwise we should
> avoid introducing a new thing which is hard to argue about its existent.

I don't want a new structure. I just want you to use the bio_vec for
spots where you are needing to use a page because you are populating a
bio_vec.

> > allocation. It would fit well into your probe routines since they are
> > all essentially passing the page, offset, and fragsz throughout the
> > code.
>
> For the current probe routines, the 'va' need to be passed, do you
> expect the 'va' to be passed by function return, double pointer, or
> new the *_vec pointer?

I would suggest doing so via the *_vec pointer. The problem as I see
it is that the existing code is exposing too much of the internals and
setting up the possibility for a system to get corrupted really
easily. At least if you are doing this with a bio_vec you can verify
that you have the correct page and offset before you move the offset
up by the length which should have been provided by the API in the
first place and not just guessed at based on what the fragsz was that
you requested.

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Yunsheng Lin]

On 2024/8/6 8:52, Alexander Duyck wrote:
> On Sun, Aug 4, 2024 at 10:00 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote:
>>
>> On 8/3/2024 1:00 AM, Alexander Duyck wrote:
>>
>>>>
>>>>>
>>>>> As far as your API extension and naming maybe you should look like
>>>>> something like bio_vec and borrow the naming from that since that is
>>>>> essentially what you are passing back and forth is essentially that
>>>>> instead of a page frag which is normally a virtual address.
>>>>
>>>> I thought about adding something like bio_vec before, but I am not sure
>>>> what you have in mind is somthing like I considered before?
>>>> Let's say that we reuse bio_vec like something below for the new APIs:
>>>>
>>>> struct bio_vec {
>>>>          struct page     *bv_page;
>>>>          void            *va;
>>>>          unsigned int    bv_len;
>>>>          unsigned int    bv_offset;
>>>> };
>>>
>>> I wasn't suggesting changing the bio_vec. I was suggesting that be
>>> what you pass as a pointer reference instead of the offset. Basically
>>> your use case is mostly just for populating bio_vec style structures
>>> anyway.
>>
>> I wasn't trying/going to reuse/change bio_vec for page_frag, I was just
>> having a hard time coming with a good new name for it.
>> The best one I came up with is pfrag_vec, but I am not sure about the
>> 'vec' as the "vec" portion of the name would suggest, iovec structures
>> tend to come in arrays, mentioned in the below article:
>> https://lwn.net/Articles/625077/
>>
>> Anther one is page_frag, which is currently in use.
>>
>> Or any better one in your mind?
> 
> I was suggesting using bio_vec, not some new structure. The general
> idea is that almost all the values you are using are exposed by that
> structure already in the case of the page based calls you were adding,
> so it makes sense to use what is there rather than reinventing the
> wheel.

Through a quick look, there seems to be at least three structs which have
similar values: struct bio_vec & struct skb_frag & struct page_frag.

As your above agrument about using bio_vec, it seems it is ok to use any
one of them as each one of them seems to have almost all the values we
are using?

Personally, my preference over them: 'struct page_frag' > 'struct skb_frag'
> 'struct bio_vec', as the naming of 'struct page_frag' seems to best match
the page_frag API, 'struct skb_frag' is the second preference because we
mostly need to fill skb frag anyway, and 'struct bio_vec' is the last
preference because it just happen to have almost all the values needed.

Is there any specific reason other than the above "almost all the values you
are using are exposed by that structure already " that you prefer bio_vec?

> 
>>>
>>>> It seems we have the below options for the new API:
>>>>
>>>> option 1, it seems like a better option from API naming point of view, but
>>>> it needs to return a bio_vec pointer to the caller, it seems we need to have
>>>> extra space for the pointer, I am not sure how we can avoid the memory waste
>>>> for sk_page_frag() case in patch 12:
>>>> struct bio_vec *page_frag_alloc_bio(struct page_frag_cache *nc,
>>>>                                      unsigned int fragsz, gfp_t gfp_mask);
>>>>
>>>> option 2, it need both the caller and callee to have a its own local space
>>>> for 'struct bio_vec ', I am not sure if passing the content instead of
>>>> the pointer of a struct through the function returning is the common pattern
>>>> and if it has any performance impact yet:
>>>> struct bio_vec page_frag_alloc_bio(struct page_frag_cache *nc,
>>>>                                     unsigned int fragsz, gfp_t gfp_mask);
>>>>
>>>> option 3, the caller passes the pointer of 'struct bio_vec ' to the callee,
>>>> and page_frag_alloc_bio() fills in the data, I am not sure what is the point
>>>> of indirect using 'struct bio_vec ' instead of passing 'va' & 'fragsz' &
>>>> 'offset' through pointers directly:
>>>> bool page_frag_alloc_bio(struct page_frag_cache *nc,
>>>>                           unsigned int fragsz, gfp_t gfp_mask, struct bio_vec *bio);
>>>>
>>>> If one of the above option is something in your mind? Yes, please be more specific
>>>> about which one is the prefer option, and why it is the prefer option than the one
>>>> introduced in this patchset?
>>>>
>>>> If no, please be more specific what that is in your mind?
>>>
>>> Option 3 is more or less what I had in mind. Basically you would
>>> return an int to indicate any errors and you would be populating a
>>> bio_vec during your allocation. In addition you would use the bio_vec
>>
>> Actually using this new bio_vec style structures does not seem to solve
>> the APIs naming issue this patch is trying to solve as my understanding,
>> as the new struct is only about passing one pointer or multi-pointers
>> from API naming perspective. It is part of the API naming, but not all
>> of it.
> 
> I have no idea what you are talking about. The issue was you were
> splitting things page_frag_alloc_va and page_frag_alloc_pg. Now it
> would be page_frag_alloc and page_frag_alloc_bio or maybe
> page_frag_fill_bio which would better explain what you are doing with
> this function.

There are three types of API as proposed in this patchset instead of
two types of API:
1. page_frag_alloc_va() returns [va].
2. page_frag_alloc_pg() returns [page, offset].
3. page_frag_alloc() returns [va] & [page, offset].

You seemed to miss that we need a third naming for the type 3 API.
Do you see type 3 API as a valid API? if yes, what naming are you
suggesting for it? if no, why it is not a valid API?

> 
>>> as a tracker of the actual fragsz so when you commit you are
>>> committing with the fragsz as it was determined at the time of putting
>>> the bio_vec together so you can theoretically catch things like if the
>>> underlying offset had somehow changed from the time you setup the
>>
>> I think we might need a stronger argument than the above to use the new
>> *vec thing other than the above debugging feature.
>>
>> I looked throught the bio_vec related info, and come along somewhat not
>> really related, but really helpful "What’s all this get us" section:
>> https://docs.kernel.org/block/biovecs.html
>>
>> So the question seems to be: what is this new struct for page_frag get
>> us?
>>
>> Generally, I am argeed with the new struct thing if it does bring us
>> something other than the above debugging feature. Otherwise we should
>> avoid introducing a new thing which is hard to argue about its existent.
> 
> I don't want a new structure. I just want you to use the bio_vec for
> spots where you are needing to use a page because you are populating a
> bio_vec.
> 
>>> allocation. It would fit well into your probe routines since they are
>>> all essentially passing the page, offset, and fragsz throughout the
>>> code.
>>
>> For the current probe routines, the 'va' need to be passed, do you
>> expect the 'va' to be passed by function return, double pointer, or
>> new the *_vec pointer?
> 
> I would suggest doing so via the *_vec pointer. The problem as I see

As your above suggestion, I can safely assume *_ve is 'struct bio_vec',
right?

I am really confused here, you just clarified that you wasn't suggesting
changing the bio_vec, and now you are suggesting passing 'va' via the
'struct bio_vec' pointer?  How is it possible with current definition of
'struct bio_vec'?

struct bio_vec {
	struct page	*bv_page;
	unsigned int	bv_len;
	unsigned int	bv_offset;
};

Or am I mising something obvious here?

> it is that the existing code is exposing too much of the internals and
> setting up the possibility for a system to get corrupted really

If most of the page_frag API callers doesn't access 'struct bio_vec'
directly and use something like bvec_iter_* API to do the accessing,
then I am agreed with the above argument.

But right now, most of the page_frag API callers are accessing 'va'
directly to do the memcpy'ing, and accessing 'page & off & len' directly
to do skb frag filling, so I am not really sure what's the point of
indirection using the 'struct bio_vec' here.

And adding 'struct bio_vec' for page_frag and accessing the value of it
directly may be against of the design choice of 'struct bio_vec', as
there seems to be no inline helper defined to access the value of
'struct bio_vec' directly in bvec.h

Re: [PATCH net-next v12 04/14] mm: page_frag: add '_va' suffix to page_frag API

[Sagi Grimberg]

Regardless of the API discussion,

The nvme-tcp bits look straight-forward:
Acked-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH net-next v12 05/14] mm: page_frag: avoid caller accessing 'page_frag_cache' directly

[Yunsheng Lin]

Use appropriate frag_page API instead of caller accessing
'page_frag_cache' directly.

CC: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/vhost/net.c             |  2 +-
 include/linux/page_frag_cache.h | 10 ++++++++++
 mm/page_frag_test.c             |  2 +-
 net/core/skbuff.c               |  6 +++---
 net/rxrpc/conn_object.c         |  4 +---
 net/rxrpc/local_object.c        |  4 +---
 net/sunrpc/svcsock.c            |  6 ++----
 7 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 6691fac01e0d..b2737dc0dc50 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -1325,7 +1325,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
 			vqs[VHOST_NET_VQ_RX]);
 
 	f->private_data = n;
-	n->pf_cache.va = NULL;
+	page_frag_cache_init(&n->pf_cache);
 
 	return 0;
 }
diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
index ef038a07925c..7c9125a9aed3 100644
--- a/include/linux/page_frag_cache.h
+++ b/include/linux/page_frag_cache.h
@@ -7,6 +7,16 @@
 #include <linux/types.h>
 #include <linux/mm_types_task.h>
 
+static inline void page_frag_cache_init(struct page_frag_cache *nc)
+{
+	nc->va = NULL;
+}
+
+static inline bool page_frag_cache_is_pfmemalloc(struct page_frag_cache *nc)
+{
+	return !!nc->pfmemalloc;
+}
+
 void page_frag_cache_drain(struct page_frag_cache *nc);
 void __page_frag_cache_drain(struct page *page, unsigned int count);
 void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
diff --git a/mm/page_frag_test.c b/mm/page_frag_test.c
index 9eaa3ab74b29..6df8d8865afe 100644
--- a/mm/page_frag_test.c
+++ b/mm/page_frag_test.c
@@ -344,7 +344,7 @@ static int __init page_frag_test_init(void)
 	u64 duration;
 	int ret;
 
-	test_frag.va = NULL;
+	page_frag_cache_init(&test_frag);
 	atomic_set(&nthreads, 2);
 	init_completion(&wait);
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 4b8acd967793..76a473b1072d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -749,14 +749,14 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev, unsigned int len,
 	if (in_hardirq() || irqs_disabled()) {
 		nc = this_cpu_ptr(&netdev_alloc_cache);
 		data = page_frag_alloc_va(nc, len, gfp_mask);
-		pfmemalloc = nc->pfmemalloc;
+		pfmemalloc = page_frag_cache_is_pfmemalloc(nc);
 	} else {
 		local_bh_disable();
 		local_lock_nested_bh(&napi_alloc_cache.bh_lock);
 
 		nc = this_cpu_ptr(&napi_alloc_cache.page);
 		data = page_frag_alloc_va(nc, len, gfp_mask);
-		pfmemalloc = nc->pfmemalloc;
+		pfmemalloc = page_frag_cache_is_pfmemalloc(nc);
 
 		local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
 		local_bh_enable();
@@ -846,7 +846,7 @@ struct sk_buff *napi_alloc_skb(struct napi_struct *napi, unsigned int len)
 		len = SKB_HEAD_ALIGN(len);
 
 		data = page_frag_alloc_va(&nc->page, len, gfp_mask);
-		pfmemalloc = nc->page.pfmemalloc;
+		pfmemalloc = page_frag_cache_is_pfmemalloc(&nc->page);
 	}
 	local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
 
diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
index 1539d315afe7..694c4df7a1a3 100644
--- a/net/rxrpc/conn_object.c
+++ b/net/rxrpc/conn_object.c
@@ -337,9 +337,7 @@ static void rxrpc_clean_up_connection(struct work_struct *work)
 	 */
 	rxrpc_purge_queue(&conn->rx_queue);
 
-	if (conn->tx_data_alloc.va)
-		__page_frag_cache_drain(virt_to_page(conn->tx_data_alloc.va),
-					conn->tx_data_alloc.pagecnt_bias);
+	page_frag_cache_drain(&conn->tx_data_alloc);
 	call_rcu(&conn->rcu, rxrpc_rcu_free_connection);
 }
 
diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
index 504453c688d7..a8cffe47cf01 100644
--- a/net/rxrpc/local_object.c
+++ b/net/rxrpc/local_object.c
@@ -452,9 +452,7 @@ void rxrpc_destroy_local(struct rxrpc_local *local)
 #endif
 	rxrpc_purge_queue(&local->rx_queue);
 	rxrpc_purge_client_connections(local);
-	if (local->tx_alloc.va)
-		__page_frag_cache_drain(virt_to_page(local->tx_alloc.va),
-					local->tx_alloc.pagecnt_bias);
+	page_frag_cache_drain(&local->tx_alloc);
 }
 
 /*
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 42d20412c1c3..4b1e87187614 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1609,7 +1609,6 @@ static void svc_tcp_sock_detach(struct svc_xprt *xprt)
 static void svc_sock_free(struct svc_xprt *xprt)
 {
 	struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
-	struct page_frag_cache *pfc = &svsk->sk_frag_cache;
 	struct socket *sock = svsk->sk_sock;
 
 	trace_svcsock_free(svsk, sock);
@@ -1619,8 +1618,7 @@ static void svc_sock_free(struct svc_xprt *xprt)
 		sockfd_put(sock);
 	else
 		sock_release(sock);
-	if (pfc->va)
-		__page_frag_cache_drain(virt_to_head_page(pfc->va),
-					pfc->pagecnt_bias);
+
+	page_frag_cache_drain(&svsk->sk_frag_cache);
 	kfree(svsk);
 }
-- 
2.33.0

Re: [PATCH net-next v12 05/14] mm: page_frag: avoid caller accessing 'page_frag_cache' directly

[Chuck Lever]

On Wed, Jul 31, 2024 at 08:44:55PM +0800, Yunsheng Lin wrote:
> Use appropriate frag_page API instead of caller accessing
> 'page_frag_cache' directly.
> 
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

For the net/sunrpc/svcsock.c hunk:

Acked-by: Chuck Lever <chuck.lever@oracle.com>


> ---
>  drivers/vhost/net.c             |  2 +-
>  include/linux/page_frag_cache.h | 10 ++++++++++
>  mm/page_frag_test.c             |  2 +-
>  net/core/skbuff.c               |  6 +++---
>  net/rxrpc/conn_object.c         |  4 +---
>  net/rxrpc/local_object.c        |  4 +---
>  net/sunrpc/svcsock.c            |  6 ++----
>  7 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 6691fac01e0d..b2737dc0dc50 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1325,7 +1325,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
>  			vqs[VHOST_NET_VQ_RX]);
>  
>  	f->private_data = n;
> -	n->pf_cache.va = NULL;
> +	page_frag_cache_init(&n->pf_cache);
>  
>  	return 0;
>  }
> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
> index ef038a07925c..7c9125a9aed3 100644
> --- a/include/linux/page_frag_cache.h
> +++ b/include/linux/page_frag_cache.h
> @@ -7,6 +7,16 @@
>  #include <linux/types.h>
>  #include <linux/mm_types_task.h>
>  
> +static inline void page_frag_cache_init(struct page_frag_cache *nc)
> +{
> +	nc->va = NULL;
> +}
> +
> +static inline bool page_frag_cache_is_pfmemalloc(struct page_frag_cache *nc)
> +{
> +	return !!nc->pfmemalloc;
> +}
> +
>  void page_frag_cache_drain(struct page_frag_cache *nc);
>  void __page_frag_cache_drain(struct page *page, unsigned int count);
>  void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
> diff --git a/mm/page_frag_test.c b/mm/page_frag_test.c
> index 9eaa3ab74b29..6df8d8865afe 100644
> --- a/mm/page_frag_test.c
> +++ b/mm/page_frag_test.c
> @@ -344,7 +344,7 @@ static int __init page_frag_test_init(void)
>  	u64 duration;
>  	int ret;
>  
> -	test_frag.va = NULL;
> +	page_frag_cache_init(&test_frag);
>  	atomic_set(&nthreads, 2);
>  	init_completion(&wait);
>  
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 4b8acd967793..76a473b1072d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -749,14 +749,14 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev, unsigned int len,
>  	if (in_hardirq() || irqs_disabled()) {
>  		nc = this_cpu_ptr(&netdev_alloc_cache);
>  		data = page_frag_alloc_va(nc, len, gfp_mask);
> -		pfmemalloc = nc->pfmemalloc;
> +		pfmemalloc = page_frag_cache_is_pfmemalloc(nc);
>  	} else {
>  		local_bh_disable();
>  		local_lock_nested_bh(&napi_alloc_cache.bh_lock);
>  
>  		nc = this_cpu_ptr(&napi_alloc_cache.page);
>  		data = page_frag_alloc_va(nc, len, gfp_mask);
> -		pfmemalloc = nc->pfmemalloc;
> +		pfmemalloc = page_frag_cache_is_pfmemalloc(nc);
>  
>  		local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
>  		local_bh_enable();
> @@ -846,7 +846,7 @@ struct sk_buff *napi_alloc_skb(struct napi_struct *napi, unsigned int len)
>  		len = SKB_HEAD_ALIGN(len);
>  
>  		data = page_frag_alloc_va(&nc->page, len, gfp_mask);
> -		pfmemalloc = nc->page.pfmemalloc;
> +		pfmemalloc = page_frag_cache_is_pfmemalloc(&nc->page);
>  	}
>  	local_unlock_nested_bh(&napi_alloc_cache.bh_lock);
>  
> diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
> index 1539d315afe7..694c4df7a1a3 100644
> --- a/net/rxrpc/conn_object.c
> +++ b/net/rxrpc/conn_object.c
> @@ -337,9 +337,7 @@ static void rxrpc_clean_up_connection(struct work_struct *work)
>  	 */
>  	rxrpc_purge_queue(&conn->rx_queue);
>  
> -	if (conn->tx_data_alloc.va)
> -		__page_frag_cache_drain(virt_to_page(conn->tx_data_alloc.va),
> -					conn->tx_data_alloc.pagecnt_bias);
> +	page_frag_cache_drain(&conn->tx_data_alloc);
>  	call_rcu(&conn->rcu, rxrpc_rcu_free_connection);
>  }
>  
> diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
> index 504453c688d7..a8cffe47cf01 100644
> --- a/net/rxrpc/local_object.c
> +++ b/net/rxrpc/local_object.c
> @@ -452,9 +452,7 @@ void rxrpc_destroy_local(struct rxrpc_local *local)
>  #endif
>  	rxrpc_purge_queue(&local->rx_queue);
>  	rxrpc_purge_client_connections(local);
> -	if (local->tx_alloc.va)
> -		__page_frag_cache_drain(virt_to_page(local->tx_alloc.va),
> -					local->tx_alloc.pagecnt_bias);
> +	page_frag_cache_drain(&local->tx_alloc);
>  }
>  
>  /*
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 42d20412c1c3..4b1e87187614 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -1609,7 +1609,6 @@ static void svc_tcp_sock_detach(struct svc_xprt *xprt)
>  static void svc_sock_free(struct svc_xprt *xprt)
>  {
>  	struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
> -	struct page_frag_cache *pfc = &svsk->sk_frag_cache;
>  	struct socket *sock = svsk->sk_sock;
>  
>  	trace_svcsock_free(svsk, sock);
> @@ -1619,8 +1618,7 @@ static void svc_sock_free(struct svc_xprt *xprt)
>  		sockfd_put(sock);
>  	else
>  		sock_release(sock);
> -	if (pfc->va)
> -		__page_frag_cache_drain(virt_to_head_page(pfc->va),
> -					pfc->pagecnt_bias);
> +
> +	page_frag_cache_drain(&svsk->sk_frag_cache);
>  	kfree(svsk);
>  }
> -- 
> 2.33.0
> 

-- 
Chuck Lever