Add xdp_update_skb_shared_info utility routine
Subsystems (5)
cover Add xdp_update_skb_shared_info utility routine
Introduce xdp_update_skb_shared_info routine to update frags array metadata from a given xdp_buffer/xdp_frame.
Add xdp_frags_tsize field to skb_shared_info.
Lorenzo Bianconi (2):
net: skbuff: add xdp_frags_tsize field to skb_shared_info
net: xdp: add xdp_update_skb_shared_info utility routine
drivers/net/ethernet/marvell/mvneta.c | 29 +++++++++++++++------------ include/linux/skbuff.h | 2 ++ include/net/xdp.h | 3 +++ net/core/xdp.c | 27 +++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 13 deletions(-)
Signature · 1 line
2.31.1
PATCH 1/2 net: skbuff: add xdp_frags_tsize field to skb_shared_info
xdp_frags_tsize field will be used to store paged frame truesize for xdp_buff/xdp_frame. In order to not increase skb_shared_info size we will use a hole due to skb_shared_info alignment.gso_type filed will be used to store paged frame size. This is a preliminary patch to properly support xdp multi-buff
--- include/linux/skbuff.h | 2 ++ 1 file changed, 2 insertions(+)
include/linux/skbuff.h +2 -0
@@ -522,6 +522,7 @@ struct skb_shared_info {unsigned short gso_segs;struct sk_buff *frag_list;struct skb_shared_hwtstamps hwtstamps; +/* used for xdp_{buff,frame} paged size */ unsigned int gso_type;u32 tskey;
@@ -529,6 +530,7 @@ struct skb_shared_info {* Warning : all fields before dataref are cleared in __alloc_skb() */ atomic_t dataref; +unsigned int xdp_frags_tsize;/* Intermediate layers must ensure that destructor_arg * remains valid until skb destructor */
Signature · 1 line
2.31.1
PATCH 2/2 net: xdp: add xdp_update_skb_shared_info utility routine
In reply to Lorenzo BianconiIntroduce xdp_update_skb_shared_info routine to update frags array metadata from a given xdp_buffer/xdp_frame. We do not need to reset frags array since it is already initialized by the driver. Rely on xdp_update_skb_shared_info in mvneta driver.
--- drivers/net/ethernet/marvell/mvneta.c | 29 +++++++++++++++------------ include/net/xdp.h | 3 +++ net/core/xdp.c | 27 +++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 13 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +16 -13
@@ -2294,18 +2294,29 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, rx_desc->buf_phys_addr =0 ;if (data_len >0 && xdp_sinfo->nr_frags <MAX_SKB_FRAGS ) { -skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags++]; +skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags];skb_frag_off_set (frag, pp->rx_offset_correction);skb_frag_size_set (frag, data_len);__skb_frag_set_page (frag, page); +/* We don't need to reset pp_recycle here. It's already set, so +* just mark fragments for recycling. +*/ +page_pool_store_mem_info (page, rxq->page_pool); + +/* first fragment */ +if (!xdp_sinfo->nr_frags) + xdp_sinfo->gso_type = *size; + xdp_sinfo->nr_frags++;/* last fragment */ if (len == *size) {struct skb_shared_info *sinfo; sinfo =xdp_get_shared_info_from_buff (xdp); + sinfo->xdp_frags_tsize = xdp_sinfo->nr_frags *PAGE_SIZE ; sinfo->nr_frags = xdp_sinfo->nr_frags; + sinfo->gso_type = xdp_sinfo->gso_type;memcpy (sinfo->frags, xdp_sinfo->frags, sinfo->nr_frags *sizeof (skb_frag_t)); }
@@ -2320,7 +2331,7 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, struct xdp_buff *xdp, u32 desc_status) {struct skb_shared_info *sinfo =xdp_get_shared_info_from_buff (xdp); -int i, num_frags = sinfo->nr_frags; +int num_frags = sinfo->nr_frags, size = sinfo->gso_type;struct sk_buff *skb; skb =build_skb (xdp->data_hard_start,PAGE_SIZE );
@@ -2333,17 +2344,9 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,skb_put (skb, xdp->data_end - xdp->data); skb->ip_summed =mvneta_rx_csum (pp, desc_status); -for (i =0 ; i < num_frags; i++) { -skb_frag_t *frag = &sinfo->frags[i]; - -skb_add_rx_frag (skb,skb_shinfo (skb)->nr_frags, -skb_frag_page (frag),skb_frag_off (frag), -skb_frag_size (frag),PAGE_SIZE ); -/* We don't need to reset pp_recycle here. It's already set, so -* just mark fragments for recycling. -*/ -page_pool_store_mem_info (skb_frag_page (frag), pool); - } +if (num_frags) +xdp_update_skb_shared_info (skb, num_frags, size, + sinfo->xdp_frags_tsize, sinfo);return skb; }
include/net/xdp.h +3 -0
@@ -164,6 +164,9 @@ void xdp_warn(const char *msg, const char *func, const int line);#define XDP_WARN (msg) xdp_warn(msg, __func__, __LINE__)struct xdp_frame *xdp_convert_zc_to_xdp_frame (struct xdp_buff *xdp); +void xdp_update_skb_shared_info (struct sk_buff *skb,int nr_frags, +int size,int truesize, +struct skb_shared_info *sinfo);struct sk_buff *__xdp_build_skb_from_frame (struct xdp_frame *xdpf,struct sk_buff *skb,struct net_device *dev);
net/core/xdp.c +27 -0
@@ -527,6 +527,33 @@ int xdp_alloc_skb_bulk(void **skbs, int n_skb, gfp_t gfp) }EXPORT_SYMBOL_GPL (xdp_alloc_skb_bulk); +void xdp_update_skb_shared_info (struct sk_buff *skb,int nr_frags, +int size,int truesize, +struct skb_shared_info *sinfo) +{ +int i; + +skb_shinfo (skb)->nr_frags = nr_frags; + + skb->len += size; + skb->data_len += size; + skb->truesize += truesize; + +if (skb->pfmemalloc) +return ; + +for (i =0 ; i < nr_frags; i++) { +struct page *page =skb_frag_page (&sinfo->frags[i]); + + page =compound_head (page); +if (page_is_pfmemalloc (page)) { + skb->pfmemalloc = true; +break ; + } + } +} +EXPORT_SYMBOL_GPL (xdp_update_skb_shared_info); +struct sk_buff *__xdp_build_skb_from_frame (struct xdp_frame *xdpf,struct sk_buff *skb,struct net_device *dev)
Signature · 1 line
2.31.1
John Fastabend
maintainerRE: [PATCH bpf-next 2/2] net: xdp: add xdp_update_skb_shared_info utility routine
Quoted 4 lines · Lorenzo Bianconi
Introduce
xdp_update_skb_shared_inforoutine to update frags array metadata from a given xdp_buffer/xdp_frame. We do not need to reset frags array since it is already initialized by the driver. Rely onxdp_update_skb_shared_infoin mvneta driver.
Some more context here would really help. I had to jump into the mvneta driver to see what is happening.
So as I read this we have a loop processing the descriptor in mvneta_rx_swbm()
mvneta_rx_swbm()
while (rx_proc < budget && rx_proc < rx_todo) {
if (rx_status & MVNETA_RXD_FIRST_DESC) ...
else {
mvneta_swbm_add_rx_fragment()
}
..
if (!rx_status & MVNETA_RXD_LAST_DESC)
continue;
..
if (xdp_prog)
mvneta_run_xdp(...)}
roughly looking like above. First question, do you ever hit !MVNETA_RXD_LAST_DESC today? I assume this is avoided by hardware setup when XDP is enabled, otherwise _run_xdp() would be broken correct? Next question, given last descriptor bit logic whats the condition to hit the code added in this patch? wouldn't we need more than 1 descriptor and then we would skip the xdp_run... sorry lost me and its probably easier to let you give the flow vs spending an hour trying to track it down.
But, in theory as you handle a hardware discriptor you can build up a set of pages using them to create a single skb rather than a skb per descriptor. But don't we know if pfmemalloc should be done while we are building the frag list? Can't se just set it vs this for loop in xdp_update_skb_shared_info(),
Quoted 10 lines
net/core/xdp.c +10
-0+for (i =0 ; i < nr_frags; i++) { +struct page *page =skb_frag_page (&sinfo->frags[i]); + + page =compound_head (page); +if (page_is_pfmemalloc (page)) { + skb->pfmemalloc = true; +break ; + } + } +}
...
Quoted 12 lines
drivers/net/ethernet/marvell/mvneta.c +1
-1@@ -2294,18 +2294,29 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, rx_desc->buf_phys_addr =0 ;if (data_len >0 && xdp_sinfo->nr_frags <MAX_SKB_FRAGS ) { -skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags++]; +skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags];skb_frag_off_set (frag, pp->rx_offset_correction);
drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, · line 2309
Quoted 10 lines
drivers/net/ethernet/marvell/mvneta.c +8
-0skb_frag_size_set (frag, data_len);__skb_frag_set_page (frag, page); +/* We don't need to reset pp_recycle here. It's already set, so +* just mark fragments for recycling. +*/ +page_pool_store_mem_info (page, rxq->page_pool); + +/* first fragment */ +if (!xdp_sinfo->nr_frags) + xdp_sinfo->gso_type = *size;
Would be nice to also change 'int size' -> 'unsigned int size' so the types matched. Presumably you really can't have a negative size.
Also how about giving gso_type a better name. xdp_sinfo->size maybe?
drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, · line 2322
Quoted 13 lines
drivers/net/ethernet/marvell/mvneta.c +3
-0+ xdp_sinfo->nr_frags++;/* last fragment */ if (len == *size) {struct skb_shared_info *sinfo; sinfo =xdp_get_shared_info_from_buff (xdp); + sinfo->xdp_frags_tsize = xdp_sinfo->nr_frags *PAGE_SIZE ; sinfo->nr_frags = xdp_sinfo->nr_frags; + sinfo->gso_type = xdp_sinfo->gso_type;memcpy (sinfo->frags, xdp_sinfo->frags, sinfo->nr_frags *sizeof (skb_frag_t)); }
Thanks,
John
Re: [PATCH bpf-next 2/2] net: xdp: add xdp_update_skb_shared_info utility routine
Quoted 8 lines
Quoted 4 lines · Lorenzo Bianconi
Introduce
xdp_update_skb_shared_inforoutine to update frags array metadata from a given xdp_buffer/xdp_frame. We do not need to reset frags array since it is already initialized by the driver. Rely onxdp_update_skb_shared_infoin mvneta driver.Some more context here would really help. I had to jump into the mvneta driver to see what is happening.
Hi John,
ack, you are right. I will add more context next time. Sorry for the noise.
Quoted 18 lines
So as I read this we have a loop processing the descriptor in
mvneta_rx_swbm()
mvneta_rx_swbm()while (rx_proc < budget && rx_proc < rx_todo) { if (rx_status & MVNETA_RXD_FIRST_DESC) ... else { mvneta_swbm_add_rx_fragment() } .. if (!rx_status & MVNETA_RXD_LAST_DESC) continue; .. if (xdp_prog) mvneta_run_xdp(...)}
Quoted 9 lines
roughly looking like above. First question, do you ever hit !MVNETA_RXD_LAST_DESC today? I assume this is avoided by hardware setup when XDP is enabled, otherwise
_run_xdp()would be broken correct? Next question, given last descriptor bit logic whats the condition to hit the code added in this patch? wouldn't we need more than 1 descriptor and then we would skip thexdp_run... sorry lost me and its probably easier to let you give the flow vs spending an hour trying to track it down.
I will point it out in the new commit log, but this is a preliminary patch for xdp multi-buff support. In the current codebase xdp_update_skb_shared_info() is run just when the NIC is not running in XDP mode (please note mvneta_swbm_add_rx_fragment() is run even if xdp_prog is NULL). When we add xdp multi-buff support, xdp_update_skb_shared_info() will run even in XDP mode since we will remove the MTU constraint.
In the current codebsae the following condition can occur in non-XDP mode if the packet is split on 3 or more descriptors (e.g. MTU 9000):
if (!(rx_status & MVNETA_RXD_LAST_DESC))
continue;
Quoted 6 lines
But, in theory as you handle a hardware discriptor you can build up a set of pages using them to create a single skb rather than a skb per descriptor. But don't we know if pfmemalloc should be done while we are building the frag list? Can't se just set it vs this for loop in
xdp_update_skb_shared_info(),
I added pfmemalloc code in xdp_update_skb_shared_info() in order to reuse it for the xdp_redirect use-case (e.g. whenever we redirect a xdp multi-buff in a veth or in a cpumap). I have a pending patch where I am using xdp_update_skb_shared_info in __xdp_build_skb_from_frame().
Quoted 25 lines
Quoted 11 lines
net/core/xdp.c +10
-0+for (i =0 ; i < nr_frags; i++) { +struct page *page =skb_frag_page (&sinfo->frags[i]); + + page =compound_head (page); +if (page_is_pfmemalloc (page)) { + skb->pfmemalloc = true; +break ; + } + } +}...
Quoted 11 lines
drivers/net/ethernet/marvell/mvneta.c +1
-1@@ -2294,18 +2294,29 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, rx_desc->buf_phys_addr =0 ;if (data_len >0 && xdp_sinfo->nr_frags <MAX_SKB_FRAGS ) { -skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags++]; +skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags];
Quoted 15 lines
drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, · line 2309
Quoted 11 lines
drivers/net/ethernet/marvell/mvneta.c +8
-0skb_frag_off_set (frag, pp->rx_offset_correction);skb_frag_size_set (frag, data_len);__skb_frag_set_page (frag, page); +/* We don't need to reset pp_recycle here. It's already set, so +* just mark fragments for recycling. +*/ +page_pool_store_mem_info (page, rxq->page_pool); + +/* first fragment */ +if (!xdp_sinfo->nr_frags) + xdp_sinfo->gso_type = *size;Would be nice to also change 'int size' -> 'unsigned int size' so the types matched. Presumably you really can't have a negative size.
ack
Quoted 1 line
Also how about giving
gso_typea better name.xdp_sinfo->sizemaybe?
I did it in this way in order to avoid adding a union in skb_shared_info. What about adding an inline helper to set/get it? e.g.
static inline u32 xdp_get_data_len(struct skb_shared_info *sinfo)
{
return sinfo->gso_type;
}static inline void xdp_set_data_len(struct skb_shared_info *sinfo, u32 datalen)
{
sinfo->gso_type = datalen;
}Regards,
Lorenzo
Quoted 19 lines
Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +1
-0+ xdp_sinfo->nr_frags++;/* last fragment */ drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, · line 2322
Quoted 10 lines
drivers/net/ethernet/marvell/mvneta.c +2
-0if (len == *size) {struct skb_shared_info *sinfo; sinfo =xdp_get_shared_info_from_buff (xdp); + sinfo->xdp_frags_tsize = xdp_sinfo->nr_frags *PAGE_SIZE ; sinfo->nr_frags = xdp_sinfo->nr_frags; + sinfo->gso_type = xdp_sinfo->gso_type;memcpy (sinfo->frags, xdp_sinfo->frags, sinfo->nr_frags *sizeof (skb_frag_t)); }Thanks,
John
John Fastabend
maintainerRe: [PATCH bpf-next 2/2] net: xdp: add xdp_update_skb_shared_info utility routine
Quoted 48 lines · Lorenzo Bianconi
Quoted 8 lines
Quoted 4 lines · Lorenzo Bianconi
Introduce
xdp_update_skb_shared_inforoutine to update frags array metadata from a given xdp_buffer/xdp_frame. We do not need to reset frags array since it is already initialized by the driver. Rely onxdp_update_skb_shared_infoin mvneta driver.Some more context here would really help. I had to jump into the mvneta driver to see what is happening.
Hi John,
ack, you are right. I will add more context next time. Sorry for the noise.
Quoted 18 lines
So as I read this we have a loop processing the descriptor in
mvneta_rx_swbm()
mvneta_rx_swbm()while (rx_proc < budget && rx_proc < rx_todo) { if (rx_status & MVNETA_RXD_FIRST_DESC) ... else { mvneta_swbm_add_rx_fragment() } .. if (!rx_status & MVNETA_RXD_LAST_DESC) continue; .. if (xdp_prog) mvneta_run_xdp(...)}
Quoted 9 lines
roughly looking like above. First question, do you ever hit !MVNETA_RXD_LAST_DESC today? I assume this is avoided by hardware setup when XDP is enabled, otherwise
_run_xdp()would be broken correct? Next question, given last descriptor bit logic whats the condition to hit the code added in this patch? wouldn't we need more than 1 descriptor and then we would skip thexdp_run... sorry lost me and its probably easier to let you give the flow vs spending an hour trying to track it down.I will point it out in the new commit log, but this is a preliminary patch for xdp multi-buff support. In the current codebase
xdp_update_skb_shared_info()is run just when the NIC is not running in XDP mode (please notemvneta_swbm_add_rx_fragment()is run even ifxdp_progis NULL). When we add xdp multi-buff support,xdp_update_skb_shared_info()will run even in XDP mode since we will remove the MTU constraint.
Quoted 5 lines
In the current codebsae the following condition can occur in non-XDP mode if the packet is split on 3 or more descriptors (e.g. MTU 9000):
if (!(rx_status &
MVNETA_RXD_LAST_DESC))
continue;
But, as is there is no caller of xdp_update_skb_shared_info() so I think we should move the these two patches into the series with the multibuf support.
Quoted 8 lines
Quoted 7 lines
But, in theory as you handle a hardware discriptor you can build up a set of pages using them to create a single skb rather than a skb per descriptor. But don't we know if pfmemalloc should be done while we are building the frag list? Can't se just set it vs this for loop in
xdp_update_skb_shared_info(),
Quoted 4 lines
I added pfmemalloc code in
xdp_update_skb_shared_info()in order to reuse it for thexdp_redirectuse-case (e.g. whenever we redirect a xdp multi-buff in a veth or in a cpumap). I have a pending patch where I am usingxdp_update_skb_shared_infoin__xdp_build_skb_from_frame().
OK, but it adds an extra for loop and the related overhead. Can we avoid this overhead and just set it from where we first know we have a compound page. Or carry some bit through and do a simpler check,
if (pfmemalloc_needed) skb->pfmemalloc = true;
I guess in the case here its building the skb so performance is maybe not as critical, but if it gets used in the redirect case then we shouldn't be doing unnecessary for loops.
Quoted 42 lines
Quoted 26 lines
Quoted 12 lines
net/core/xdp.c +10
-0+for (i =0 ; i < nr_frags; i++) { +struct page *page =skb_frag_page (&sinfo->frags[i]); + + page =compound_head (page); +if (page_is_pfmemalloc (page)) { + skb->pfmemalloc = true; +break ; + } + } +}...
Quoted 11 lines
drivers/net/ethernet/marvell/mvneta.c +1
-1@@ -2294,18 +2294,29 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, rx_desc->buf_phys_addr =0 ;if (data_len >0 && xdp_sinfo->nr_frags <MAX_SKB_FRAGS ) { -skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags++]; +skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags];Quoted 15 lines
drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, · line 2309
Quoted 11 lines
drivers/net/ethernet/marvell/mvneta.c +8
-0skb_frag_off_set (frag, pp->rx_offset_correction);skb_frag_size_set (frag, data_len);__skb_frag_set_page (frag, page); +/* We don't need to reset pp_recycle here. It's already set, so +* just mark fragments for recycling. +*/ +page_pool_store_mem_info (page, rxq->page_pool); + +/* first fragment */ +if (!xdp_sinfo->nr_frags) + xdp_sinfo->gso_type = *size;Would be nice to also change 'int size' -> 'unsigned int size' so the types matched. Presumably you really can't have a negative size.
Quoted 6 lines
ack
Quoted 1 line
Also how about giving
gso_typea better name.xdp_sinfo->sizemaybe?I did it in this way in order to avoid adding a union in
skb_shared_info. What about adding an inline helper to set/get it? e.g.
What was wrong with the union?
Quoted 13 lines
static inline u32
xdp_get_data_len(struct skb_shared_info *sinfo){ return sinfo->gso_type; }static inline void
xdp_set_data_len(struct skb_shared_info *sinfo, u32 datalen){ sinfo->gso_type = datalen; }Regards,
Lorenzo
Re: [PATCH bpf-next 2/2] net: xdp: add xdp_update_skb_shared_info utility routine
Quoted 52 lines
Quoted 51 lines · Lorenzo Bianconi
Quoted 8 lines
Quoted 4 lines · Lorenzo Bianconi
Introduce
xdp_update_skb_shared_inforoutine to update frags array metadata from a given xdp_buffer/xdp_frame. We do not need to reset frags array since it is already initialized by the driver. Rely onxdp_update_skb_shared_infoin mvneta driver.Some more context here would really help. I had to jump into the mvneta driver to see what is happening.
Hi John,
ack, you are right. I will add more context next time. Sorry for the noise.
Quoted 18 lines
So as I read this we have a loop processing the descriptor in
mvneta_rx_swbm()
mvneta_rx_swbm()while (rx_proc < budget && rx_proc < rx_todo) { if (rx_status & MVNETA_RXD_FIRST_DESC) ... else { mvneta_swbm_add_rx_fragment() } .. if (!rx_status & MVNETA_RXD_LAST_DESC) continue; .. if (xdp_prog) mvneta_run_xdp(...)}
Quoted 9 lines
roughly looking like above. First question, do you ever hit !MVNETA_RXD_LAST_DESC today? I assume this is avoided by hardware setup when XDP is enabled, otherwise
_run_xdp()would be broken correct? Next question, given last descriptor bit logic whats the condition to hit the code added in this patch? wouldn't we need more than 1 descriptor and then we would skip thexdp_run... sorry lost me and its probably easier to let you give the flow vs spending an hour trying to track it down.I will point it out in the new commit log, but this is a preliminary patch for xdp multi-buff support. In the current codebase
xdp_update_skb_shared_info()is run just when the NIC is not running in XDP mode (please notemvneta_swbm_add_rx_fragment()is run even ifxdp_progis NULL). When we add xdp multi-buff support,xdp_update_skb_shared_info()will run even in XDP mode since we will remove the MTU constraint.In the current codebsae the following condition can occur in non-XDP mode if the packet is split on 3 or more descriptors (e.g. MTU 9000):
Quoted 6 lines
Quoted 2 lines
if (!(rx_status &
MVNETA_RXD_LAST_DESC))
continue;But, as is there is no caller of
xdp_update_skb_shared_info()so I think we should move the these two patches into the series with the multibuf support.
mvneta is currently using it building the skb in mvneta_swbm_build_skb() running in non-xdp mode but I am fine merging this series in the multi-buff one.
Quoted 19 lines
Quoted 9 lines
Quoted 8 lines
But, in theory as you handle a hardware discriptor you can build up a set of pages using them to create a single skb rather than a skb per descriptor. But don't we know if pfmemalloc should be done while we are building the frag list? Can't se just set it vs this for loop in
xdp_update_skb_shared_info(),Quoted 4 lines
I added pfmemalloc code in
xdp_update_skb_shared_info()in order to reuse it for thexdp_redirectuse-case (e.g. whenever we redirect a xdp multi-buff in a veth or in a cpumap). I have a pending patch where I am usingxdp_update_skb_shared_infoin__xdp_build_skb_from_frame().OK, but it adds an extra for loop and the related overhead. Can we avoid this overhead and just set it from where we first know we have a compound page. Or carry some bit through and do a simpler check,
Quoted 5 lines
if (
pfmemalloc_needed)skb->pfmemalloc= true;I guess in the case here its building the skb so performance is maybe not as critical, but if it gets used in the redirect case then we shouldn't be doing unnecessary for loops.
doing so every driver will need to take care of it building the xdp_buff. Does it work to do it since probably multi-buff is not critical for performance?
In order to support xdp_redirect we need to save this info in xdp_buff/xdp_frame, maybe in the flag field added in xdp multi-buff series.
Quoted 43 lines
Quoted 43 lines
Quoted 42 lines
Quoted 13 lines
net/core/xdp.c +10
-0+for (i =0 ; i < nr_frags; i++) { +struct page *page =skb_frag_page (&sinfo->frags[i]); + + page =compound_head (page); +if (page_is_pfmemalloc (page)) { + skb->pfmemalloc = true; +break ; + } + } +}...
Quoted 12 lines
drivers/net/ethernet/marvell/mvneta.c +1
-1@@ -2294,18 +2294,29 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, rx_desc->buf_phys_addr =0 ;if (data_len >0 && xdp_sinfo->nr_frags <MAX_SKB_FRAGS ) { -skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags++]; +skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags];skb_frag_off_set (frag, pp->rx_offset_correction);drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, · line 2309
Quoted 10 lines
drivers/net/ethernet/marvell/mvneta.c +8
-0skb_frag_size_set (frag, data_len);__skb_frag_set_page (frag, page); +/* We don't need to reset pp_recycle here. It's already set, so +* just mark fragments for recycling. +*/ +page_pool_store_mem_info (page, rxq->page_pool); + +/* first fragment */ +if (!xdp_sinfo->nr_frags) + xdp_sinfo->gso_type = *size;Would be nice to also change 'int size' -> 'unsigned int size' so the types matched. Presumably you really can't have a negative size.
Quoted 8 lines
Quoted 6 lines
ack
Quoted 1 line
Also how about giving
gso_typea better name.xdp_sinfo->sizemaybe?I did it in this way in order to avoid adding a union in
skb_shared_info. What about adding an inline helper to set/get it? e.g.What was wrong with the union?
Alex requested to use gso_* fields already there (the union was in the previous version I sent).
Regards,
Lorenzo
Quoted 15 lines
Quoted 14 lines
static inline u32
xdp_get_data_len(struct skb_shared_info *sinfo){ return sinfo->gso_type; }static inline void
xdp_set_data_len(struct skb_shared_info *sinfo, u32 datalen){ sinfo->gso_type = datalen; }Regards,
Lorenzo
John Fastabend
maintainerRe: [PATCH bpf-next 2/2] net: xdp: add xdp_update_skb_shared_info utility routine
Quoted 55 lines · Lorenzo Bianconi
Quoted 55 lines
Quoted 53 lines · Lorenzo Bianconi
Quoted 8 lines
Quoted 4 lines · Lorenzo Bianconi
Introduce
xdp_update_skb_shared_inforoutine to update frags array metadata from a given xdp_buffer/xdp_frame. We do not need to reset frags array since it is already initialized by the driver. Rely onxdp_update_skb_shared_infoin mvneta driver.Some more context here would really help. I had to jump into the mvneta driver to see what is happening.
Hi John,
ack, you are right. I will add more context next time. Sorry for the noise.
Quoted 18 lines
So as I read this we have a loop processing the descriptor in
mvneta_rx_swbm()
mvneta_rx_swbm()while (rx_proc < budget && rx_proc < rx_todo) { if (rx_status & MVNETA_RXD_FIRST_DESC) ... else { mvneta_swbm_add_rx_fragment() } .. if (!rx_status & MVNETA_RXD_LAST_DESC) continue; .. if (xdp_prog) mvneta_run_xdp(...)}
Quoted 9 lines
roughly looking like above. First question, do you ever hit !MVNETA_RXD_LAST_DESC today? I assume this is avoided by hardware setup when XDP is enabled, otherwise
_run_xdp()would be broken correct? Next question, given last descriptor bit logic whats the condition to hit the code added in this patch? wouldn't we need more than 1 descriptor and then we would skip thexdp_run... sorry lost me and its probably easier to let you give the flow vs spending an hour trying to track it down.I will point it out in the new commit log, but this is a preliminary patch for xdp multi-buff support. In the current codebase
xdp_update_skb_shared_info()is run just when the NIC is not running in XDP mode (please notemvneta_swbm_add_rx_fragment()is run even ifxdp_progis NULL). When we add xdp multi-buff support,xdp_update_skb_shared_info()will run even in XDP mode since we will remove the MTU constraint.In the current codebsae the following condition can occur in non-XDP mode if the packet is split on 3 or more descriptors (e.g. MTU 9000):
if (!(rx_status &
MVNETA_RXD_LAST_DESC))
continue;
Quoted 7 lines
Quoted 3 lines
But, as is there is no caller of
xdp_update_skb_shared_info()so I think we should move the these two patches into the series with the multibuf support.mvneta is currently using it building the skb in
mvneta_swbm_build_skb()running in non-xdp mode but I am fine merging this series in the multi-buff one.
My preference is to add it where it will be used. So in the multi-buf series.
Quoted 22 lines
Quoted 22 lines
Quoted 10 lines
Quoted 9 lines
But, in theory as you handle a hardware discriptor you can build up a set of pages using them to create a single skb rather than a skb per descriptor. But don't we know if pfmemalloc should be done while we are building the frag list? Can't se just set it vs this for loop in
xdp_update_skb_shared_info(),Quoted 4 lines
I added pfmemalloc code in
xdp_update_skb_shared_info()in order to reuse it for thexdp_redirectuse-case (e.g. whenever we redirect a xdp multi-buff in a veth or in a cpumap). I have a pending patch where I am usingxdp_update_skb_shared_infoin__xdp_build_skb_from_frame().OK, but it adds an extra for loop and the related overhead. Can we avoid this overhead and just set it from where we first know we have a compound page. Or carry some bit through and do a simpler check,
if (
pfmemalloc_needed)skb->pfmemalloc= true;
Quoted 7 lines
Quoted 3 lines
I guess in the case here its building the skb so performance is maybe not as critical, but if it gets used in the redirect case then we shouldn't be doing unnecessary for loops.
doing so every driver will need to take care of it building the
xdp_buff. Does it work to do it since probably multi-buff is not critical for performance?
OK, but I think we need to improve performance in some of the 100Gbps drivers. Work is in progress so any thing that has potential to slow things down again I want to call out. I agree this might be OK and only matters for nr_frags case.
Quoted 2 lines
In order to support
xdp_redirectwe need to save this info in xdp_buff/xdp_frame, maybe in the flag field added in xdp multi-buff series.
Yeah I think that would work better if possible.
Quoted 48 lines
Quoted 48 lines
Quoted 48 lines
Quoted 28 lines
Quoted 14 lines
net/core/xdp.c +10
-0+for (i =0 ; i < nr_frags; i++) { +struct page *page =skb_frag_page (&sinfo->frags[i]); + + page =compound_head (page); +if (page_is_pfmemalloc (page)) { + skb->pfmemalloc = true; +break ; + } + } +}...
Quoted 11 lines
drivers/net/ethernet/marvell/mvneta.c +1
-1@@ -2294,18 +2294,29 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, rx_desc->buf_phys_addr =0 ;if (data_len >0 && xdp_sinfo->nr_frags <MAX_SKB_FRAGS ) { -skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags++]; +skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags];Quoted 15 lines
drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, · line 2309
Quoted 11 lines
drivers/net/ethernet/marvell/mvneta.c +8
-0skb_frag_off_set (frag, pp->rx_offset_correction);skb_frag_size_set (frag, data_len);__skb_frag_set_page (frag, page); +/* We don't need to reset pp_recycle here. It's already set, so +* just mark fragments for recycling. +*/ +page_pool_store_mem_info (page, rxq->page_pool); + +/* first fragment */ +if (!xdp_sinfo->nr_frags) + xdp_sinfo->gso_type = *size;Would be nice to also change 'int size' -> 'unsigned int size' so the types matched. Presumably you really can't have a negative size.
ack
Quoted 1 line
Also how about giving
gso_typea better name.xdp_sinfo->sizemaybe?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I did it in this way in order to avoid adding a union in
skb_shared_info. What about adding an inline helper to set/get it? e.g.What was wrong with the union?
Alex requested to use gso_* fields already there (the union was in the previous version I sent).
@Alex, I think you were just saying union the gso_size field not the tskey field. Anyways its a fairly small nit on my side I don't care much either way.
Quoted 3 lines
Regards,
Lorenzo