From: Gerhard Engleder <hidden> Date: 2022-12-08 05:41:11
Implement XDP support for tsnep driver. I tried to follow existing
drivers like igb/igc as far as possible. Some prework was already done
in previous patch series, so in this series only actual XDP stuff is
included.
Thanks for the NetDev 0x14 slides "Add XDP support on a NIC driver".
v2:
- move tsnep_xdp_xmit_back() to commit where it is used (Paolo Abeni)
- remove inline from tsnep_rx_offset() (Paolo Abeni)
- remove inline from tsnep_rx_offset_xdp() (Paolo Abeni)
- simplify tsnep_xdp_run_prog() call by moving xdp_status update to it (Paolo Abeni)
Gerhard Engleder (6):
tsnep: Add adapter down state
tsnep: Add XDP TX support
tsnep: Support XDP BPF program setup
tsnep: Prepare RX buffer for XDP support
tsnep: Add RX queue info for XDP support
tsnep: Add XDP RX support
drivers/net/ethernet/engleder/Makefile | 2 +-
drivers/net/ethernet/engleder/tsnep.h | 31 +-
drivers/net/ethernet/engleder/tsnep_main.c | 423 +++++++++++++++++++--
drivers/net/ethernet/engleder/tsnep_xdp.c | 27 ++
4 files changed, 453 insertions(+), 30 deletions(-)
create mode 100644 drivers/net/ethernet/engleder/tsnep_xdp.c
--
2.30.2
From: Gerhard Engleder <hidden> Date: 2022-12-08 05:41:12
Add adapter state with flag for down state. This flag will be used by
the XDP TX path to deny TX if adapter is down.
Signed-off-by: Gerhard Engleder <redacted>
---
drivers/net/ethernet/engleder/tsnep.h | 1 +
drivers/net/ethernet/engleder/tsnep_main.c | 11 +++++++++++
2 files changed, 12 insertions(+)
@@ -310,10 +310,11 @@ static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,structtsnep_tx_entry*entry=&tx->entry[index];entry->properties=0;-if(entry->skb){+if(entry->skb||entry->xdpf){entry->properties=length&TSNEP_DESC_LENGTH_MASK;entry->properties|=TSNEP_DESC_INTERRUPT_FLAG;-if(skb_shinfo(entry->skb)->tx_flags&SKBTX_IN_PROGRESS)+if(entry->type==TSNEP_TX_TYPE_SKB&&+skb_shinfo(entry->skb)->tx_flags&SKBTX_IN_PROGRESS)entry->properties|=TSNEP_DESC_EXTENDED_WRITEBACK_FLAG;/* toggle user flag to prevent false acknowledge
@@ -400,6 +401,8 @@ static int tsnep_tx_map(struct sk_buff *skb, struct tsnep_tx *tx, int count)entry->desc->tx=__cpu_to_le64(dma);+entry->type=TSNEP_TX_TYPE_SKB;+map_len+=len;}
@@ -417,12 +420,13 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)entry=&tx->entry[(index+i)%TSNEP_RING_SIZE];if(entry->len){-if(i==0)+if(i==0&&entry->type==TSNEP_TX_TYPE_SKB)dma_unmap_single(dmadev,dma_unmap_addr(entry,dma),dma_unmap_len(entry,len),DMA_TO_DEVICE);-else+elseif(entry->type==TSNEP_TX_TYPE_SKB||+entry->type==TSNEP_TX_TYPE_XDP_NDO)dma_unmap_page(dmadev,dma_unmap_addr(entry,dma),dma_unmap_len(entry,len),
@@ -505,6 +509,122 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,returnNETDEV_TX_OK;}+staticinttsnep_xdp_tx_map(structxdp_frame*xdpf,structtsnep_tx*tx,+structskb_shared_info*shinfo,intcount,+booldma_map)+{+structdevice*dmadev=tx->adapter->dmadev;+skb_frag_t*frag;+unsignedintlen;+structtsnep_tx_entry*entry;+void*data;+structpage*page;+dma_addr_tdma;+intmap_len=0;+inti;++frag=NULL;+len=xdpf->len;+for(i=0;i<count;i++){+entry=&tx->entry[(tx->write+i)%TSNEP_RING_SIZE];+if(dma_map){+data=unlikely(frag)?skb_frag_address(frag):+xdpf->data;+dma=dma_map_single(dmadev,data,len,DMA_TO_DEVICE);+if(dma_mapping_error(dmadev,dma))+return-ENOMEM;++entry->type=TSNEP_TX_TYPE_XDP_NDO;+}else{+page=unlikely(frag)?skb_frag_page(frag):+virt_to_page(xdpf->data);+dma=page_pool_get_dma_addr(page);+if(unlikely(frag))+dma+=skb_frag_off(frag);+else+dma+=sizeof(*xdpf)+xdpf->headroom;+dma_sync_single_for_device(dmadev,dma,len,+DMA_BIDIRECTIONAL);++entry->type=TSNEP_TX_TYPE_XDP_TX;+}++entry->len=len;+dma_unmap_addr_set(entry,dma,dma);++entry->desc->tx=__cpu_to_le64(dma);++map_len+=len;++if((i+1)<count){+frag=&shinfo->frags[i];+len=skb_frag_size(frag);+}+}++returnmap_len;+}++/* This function requires __netif_tx_lock is held by the caller. */+staticinttsnep_xdp_xmit_frame_ring(structxdp_frame*xdpf,+structtsnep_tx*tx,booldma_map)+{+structskb_shared_info*shinfo=xdp_get_shared_info_from_frame(xdpf);+unsignedlongflags;+intcount=1;+structtsnep_tx_entry*entry;+intlength;+inti;+intretval;++if(unlikely(xdp_frame_has_frags(xdpf)))+count+=shinfo->nr_frags;++spin_lock_irqsave(&tx->lock,flags);++if(tsnep_tx_desc_available(tx)<(MAX_SKB_FRAGS+1+count)){+/* prevent full TX ring due to XDP */+spin_unlock_irqrestore(&tx->lock,flags);++return-EBUSY;+}++entry=&tx->entry[tx->write];+entry->xdpf=xdpf;++retval=tsnep_xdp_tx_map(xdpf,tx,shinfo,count,dma_map);+if(retval<0){+tsnep_tx_unmap(tx,tx->write,count);+entry->xdpf=NULL;++tx->dropped++;++spin_unlock_irqrestore(&tx->lock,flags);++netdev_err(tx->adapter->netdev,"XDP TX DMA map failed\n");++returnretval;+}+length=retval;++for(i=0;i<count;i++)+tsnep_tx_activate(tx,(tx->write+i)%TSNEP_RING_SIZE,length,+i==(count-1));+tx->write=(tx->write+count)%TSNEP_RING_SIZE;++/* descriptor properties shall be valid before hardware is notified */+dma_wmb();++spin_unlock_irqrestore(&tx->lock,flags);++return0;+}++staticvoidtsnep_xdp_xmit_flush(structtsnep_tx*tx)+{+iowrite32(TSNEP_CONTROL_TX_ENABLE,tx->addr+TSNEP_CONTROL);+}+staticbooltsnep_tx_poll(structtsnep_tx*tx,intnapi_budget){unsignedlongflags;
@@ -512,6 +632,11 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)structtsnep_tx_entry*entry;intcount;intlength;+structxdp_frame_bulkbq;++xdp_frame_bulk_init(&bq);++rcu_read_lock();/* need for xdp_return_frame_bulk */spin_lock_irqsave(&tx->lock,flags);
@@ -1335,6 +1481,47 @@ static ktime_t tsnep_netdev_get_tstamp(struct net_device *netdev,returnns_to_ktime(timestamp);}+staticinttsnep_netdev_xdp_xmit(structnet_device*dev,intn,+structxdp_frame**xdp,u32flags)+{+structtsnep_adapter*adapter=netdev_priv(dev);+intcpu=smp_processor_id();+intqueue;+structnetdev_queue*nq;+intnxmit=0;+inti;+intretval;++if(unlikely(test_bit(__TSNEP_DOWN,&adapter->state)))+return-ENETDOWN;++if(unlikely(flags&~XDP_XMIT_FLAGS_MASK))+return-EINVAL;++queue=cpu%adapter->num_tx_queues;+nq=netdev_get_tx_queue(adapter->netdev,queue);++__netif_tx_lock(nq,cpu);++/* Avoid transmit queue timeout since we share it with the slow path */+txq_trans_cond_update(nq);++for(i=0;i<n;i++){+retval=tsnep_xdp_xmit_frame_ring(xdp[i],&adapter->tx[queue],true);+if(retval)+break;++nxmit++;+}++if(flags&XDP_XMIT_FLUSH)+tsnep_xdp_xmit_flush(&adapter->tx[queue]);++__netif_tx_unlock(nq);++returnnxmit;+}+staticconststructnet_device_opstsnep_netdev_ops={.ndo_open=tsnep_netdev_open,.ndo_stop=tsnep_netdev_close,
From: Gerhard Engleder <hidden> Date: 2022-12-08 05:41:15
Reserve XDP_PACKET_HEADROOM in front of RX buffer if XDP is enabled.
Also set DMA direction properly in this case.
Signed-off-by: Gerhard Engleder <redacted>
---
drivers/net/ethernet/engleder/tsnep_main.c | 31 +++++++++++++++-------
1 file changed, 22 insertions(+), 9 deletions(-)
@@ -983,14 +995,14 @@ static struct sk_buff *tsnep_build_skb(struct tsnep_rx *rx, struct page *page,returnNULL;/* update pointers within the skb to store the data */-skb_reserve(skb,TSNEP_SKB_PAD+TSNEP_RX_INLINE_METADATA_SIZE);+skb_reserve(skb,tsnep_rx_offset(rx)+TSNEP_RX_INLINE_METADATA_SIZE);__skb_put(skb,length-TSNEP_RX_INLINE_METADATA_SIZE-ETH_FCS_LEN);if(rx->adapter->hwtstamp_config.rx_filter==HWTSTAMP_FILTER_ALL){structskb_shared_hwtstamps*hwtstamps=skb_hwtstamps(skb);structtsnep_rx_inline*rx_inline=(structtsnep_rx_inline*)(page_address(page)+-TSNEP_SKB_PAD);+tsnep_rx_offset(rx));skb_shinfo(skb)->tx_flags|=SKBTX_HW_TSTAMP_NETDEV;
From: Gerhard Engleder <hidden> Date: 2022-12-08 05:41:23
If BPF program is set up, then run BPF program for every received frame
and execute the selected action.
Test results with A53 1.2GHz:
XDP_DROP (samples/bpf/xdp1)
proto 17: 883878 pkt/s
XDP_TX (samples/bpf/xdp2)
proto 17: 255693 pkt/s
XDP_REDIRECT (samples/bpf/xdpsock)
sock0@eth2:0 rxdrop xdp-drv
pps pkts 1.00
rx 855,582 5,404,523
tx 0 0
XDP_REDIRECT (samples/bpf/xdp_redirect)
eth2->eth1 613,267 rx/s 0 err,drop/s 613,272 xmit/s
Signed-off-by: Gerhard Engleder <redacted>
---
drivers/net/ethernet/engleder/tsnep_main.c | 126 +++++++++++++++++++++
1 file changed, 126 insertions(+)
@@ -626,6 +630,33 @@ static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)iowrite32(TSNEP_CONTROL_TX_ENABLE,tx->addr+TSNEP_CONTROL);}+staticinttsnep_xdp_xmit_back(structtsnep_adapter*adapter,+structxdp_buff*xdp)+{+structxdp_frame*xdpf=xdp_convert_buff_to_frame(xdp);+intcpu=smp_processor_id();+intqueue;+structnetdev_queue*nq;+intretval;++if(unlikely(!xdpf))+return-EFAULT;++queue=cpu%adapter->num_tx_queues;+nq=netdev_get_tx_queue(adapter->netdev,queue);++__netif_tx_lock(nq,cpu);++/* Avoid transmit queue timeout since we share it with the slow path */+txq_trans_cond_update(nq);++retval=tsnep_xdp_xmit_frame_ring(xdpf,&adapter->tx[queue],false);++__netif_tx_unlock(nq);++returnretval;+}+staticbooltsnep_tx_poll(structtsnep_tx*tx,intnapi_budget){unsignedlongflags;
@@ -792,6 +823,11 @@ static unsigned int tsnep_rx_offset(struct tsnep_rx *rx)returnTSNEP_SKB_PAD;}+staticunsignedinttsnep_rx_offset_xdp(void)+{+returnXDP_PACKET_HEADROOM;+}+staticvoidtsnep_rx_ring_cleanup(structtsnep_rx*rx){structdevice*dmadev=rx->adapter->dmadev;
@@ -997,6 +1033,67 @@ static int tsnep_rx_refill(struct tsnep_rx *rx, int count, bool reuse)returni;}+staticbooltsnep_xdp_run_prog(structtsnep_rx*rx,structbpf_prog*prog,+structxdp_buff*xdp,int*status)+{+unsignedintlength;+unsignedintsync;+u32act;++length=xdp->data_end-xdp->data_hard_start-tsnep_rx_offset_xdp();++act=bpf_prog_run_xdp(prog,xdp);++/* Due xdp_adjust_tail: DMA sync for_device cover max len CPU touch */+sync=xdp->data_end-xdp->data_hard_start-tsnep_rx_offset_xdp();+sync=max(sync,length);++switch(act){+caseXDP_PASS:+returnfalse;+caseXDP_TX:+if(tsnep_xdp_xmit_back(rx->adapter,xdp)<0)+gotoout_failure;+*status|=TSNEP_XDP_TX;+returntrue;+caseXDP_REDIRECT:+if(xdp_do_redirect(rx->adapter->netdev,xdp,prog)<0)+gotoout_failure;+*status|=TSNEP_XDP_REDIRECT;+returntrue;+default:+bpf_warn_invalid_xdp_action(rx->adapter->netdev,prog,act);+fallthrough;+caseXDP_ABORTED:+out_failure:+trace_xdp_exception(rx->adapter->netdev,prog,act);+fallthrough;+caseXDP_DROP:+page_pool_put_page(rx->page_pool,virt_to_head_page(xdp->data),+sync,true);+returntrue;+}+}++staticvoidtsnep_finalize_xdp(structtsnep_adapter*adapter,intstatus)+{+intcpu=smp_processor_id();+intqueue;+structnetdev_queue*nq;++if(status&TSNEP_XDP_TX){+queue=cpu%adapter->num_tx_queues;+nq=netdev_get_tx_queue(adapter->netdev,queue);++__netif_tx_lock(nq,cpu);+tsnep_xdp_xmit_flush(&adapter->tx[queue]);+__netif_tx_unlock(nq);+}++if(status&TSNEP_XDP_REDIRECT)+xdp_do_flush();+}+staticstructsk_buff*tsnep_build_skb(structtsnep_rx*rx,structpage*page,intlength){
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Date: 2022-12-08 13:41:06
On Thu, Dec 08, 2022 at 06:40:45AM +0100, Gerhard Engleder wrote:
quoted hunk
If BPF program is set up, then run BPF program for every received frame
and execute the selected action.
Test results with A53 1.2GHz:
XDP_DROP (samples/bpf/xdp1)
proto 17: 883878 pkt/s
XDP_TX (samples/bpf/xdp2)
proto 17: 255693 pkt/s
XDP_REDIRECT (samples/bpf/xdpsock)
sock0@eth2:0 rxdrop xdp-drv
pps pkts 1.00
rx 855,582 5,404,523
tx 0 0
XDP_REDIRECT (samples/bpf/xdp_redirect)
eth2->eth1 613,267 rx/s 0 err,drop/s 613,272 xmit/s
Signed-off-by: Gerhard Engleder <redacted>
---
drivers/net/ethernet/engleder/tsnep_main.c | 126 +++++++++++++++++++++
1 file changed, 126 insertions(+)
@@ -626,6 +630,33 @@ static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)iowrite32(TSNEP_CONTROL_TX_ENABLE,tx->addr+TSNEP_CONTROL);}+staticinttsnep_xdp_xmit_back(structtsnep_adapter*adapter,+structxdp_buff*xdp)+{+structxdp_frame*xdpf=xdp_convert_buff_to_frame(xdp);+intcpu=smp_processor_id();+intqueue;+structnetdev_queue*nq;+intretval;++if(unlikely(!xdpf))+return-EFAULT;++queue=cpu%adapter->num_tx_queues;+nq=netdev_get_tx_queue(adapter->netdev,queue);++__netif_tx_lock(nq,cpu);++/* Avoid transmit queue timeout since we share it with the slow path */+txq_trans_cond_update(nq);++retval=tsnep_xdp_xmit_frame_ring(xdpf,&adapter->tx[queue],false);++__netif_tx_unlock(nq);++returnretval;+}+staticbooltsnep_tx_poll(structtsnep_tx*tx,intnapi_budget){unsignedlongflags;
@@ -792,6 +823,11 @@ static unsigned int tsnep_rx_offset(struct tsnep_rx *rx)returnTSNEP_SKB_PAD;}+staticunsignedinttsnep_rx_offset_xdp(void)+{+returnXDP_PACKET_HEADROOM;+}
@@ -997,6 +1033,67 @@ static int tsnep_rx_refill(struct tsnep_rx *rx, int count, bool reuse) return i; }+static bool tsnep_xdp_run_prog(struct tsnep_rx *rx, struct bpf_prog *prog,+ struct xdp_buff *xdp, int *status)+{+ unsigned int length;+ unsigned int sync;+ u32 act;++ length = xdp->data_end - xdp->data_hard_start - tsnep_rx_offset_xdp();
could this be xdp->data_end - xdp->data - TSNEP_RX_INLINE_METADATA_SIZE ?
Can you tell a bit more about that metadata macro that you have to handle
by yourself all the time? would be good to tell about the impact on
data_meta since you're not configuring it on xdp_prepare_buff().
+
+ act = bpf_prog_run_xdp(prog, xdp);
+
+ /* Due xdp_adjust_tail: DMA sync for_device cover max len CPU touch */
+ sync = xdp->data_end - xdp->data_hard_start - tsnep_rx_offset_xdp();
+ sync = max(sync, length);
+
+ switch (act) {
+ case XDP_PASS:
+ return false;
+ case XDP_TX:
+ if (tsnep_xdp_xmit_back(rx->adapter, xdp) < 0)
+ goto out_failure;
+ *status |= TSNEP_XDP_TX;
+ return true;
+ case XDP_REDIRECT:
+ if (xdp_do_redirect(rx->adapter->netdev, xdp, prog) < 0)
+ goto out_failure;
+ *status |= TSNEP_XDP_REDIRECT;
+ return true;
+ default:
+ bpf_warn_invalid_xdp_action(rx->adapter->netdev, prog, act);
+ fallthrough;
+ case XDP_ABORTED:
+out_failure:
+ trace_xdp_exception(rx->adapter->netdev, prog, act);
+ fallthrough;
+ case XDP_DROP:
+ page_pool_put_page(rx->page_pool, virt_to_head_page(xdp->data),
+ sync, true);
+ return true;
+ }
+}
+
+static void tsnep_finalize_xdp(struct tsnep_adapter *adapter, int status)
+{
+ int cpu = smp_processor_id();
+ int queue;
+ struct netdev_queue *nq;
@@ -417,12 +420,13 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count) entry = &tx->entry[(index + i) % TSNEP_RING_SIZE]; if (entry->len) {- if (i == 0)+ if (i == 0 && entry->type == TSNEP_TX_TYPE_SKB) dma_unmap_single(dmadev, dma_unmap_addr(entry, dma), dma_unmap_len(entry, len), DMA_TO_DEVICE);- else+ else if (entry->type == TSNEP_TX_TYPE_SKB ||+ entry->type == TSNEP_TX_TYPE_XDP_NDO) dma_unmap_page(dmadev, dma_unmap_addr(entry, dma), dma_unmap_len(entry, len),
@@ -505,6 +509,122 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb, return NETDEV_TX_OK; }+static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,+ struct skb_shared_info *shinfo, int count,+ bool dma_map)+{+ struct device *dmadev = tx->adapter->dmadev;+ skb_frag_t *frag;+ unsigned int len;+ struct tsnep_tx_entry *entry;+ void *data;+ struct page *page;+ dma_addr_t dma;+ int map_len = 0;+ int i;++ frag = NULL;+ len = xdpf->len;+ for (i = 0; i < count; i++) {+ entry = &tx->entry[(tx->write + i) % TSNEP_RING_SIZE];+ if (dma_map) {+ data = unlikely(frag) ? skb_frag_address(frag) :+ xdpf->data;+ dma = dma_map_single(dmadev, data, len, DMA_TO_DEVICE);+ if (dma_mapping_error(dmadev, dma))+ return -ENOMEM;++ entry->type = TSNEP_TX_TYPE_XDP_NDO;+ } else {+ page = unlikely(frag) ? skb_frag_page(frag) :+ virt_to_page(xdpf->data);+ dma = page_pool_get_dma_addr(page);+ if (unlikely(frag))+ dma += skb_frag_off(frag);+ else+ dma += sizeof(*xdpf) + xdpf->headroom;+ dma_sync_single_for_device(dmadev, dma, len,+ DMA_BIDIRECTIONAL);++ entry->type = TSNEP_TX_TYPE_XDP_TX;+ }++ entry->len = len;+ dma_unmap_addr_set(entry, dma, dma);++ entry->desc->tx = __cpu_to_le64(dma);++ map_len += len;++ if ((i + 1) < count) {+ frag = &shinfo->frags[i];+ len = skb_frag_size(frag);+ }+ }++ return map_len;+}++/* This function requires __netif_tx_lock is held by the caller. */+static int tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,+ struct tsnep_tx *tx, bool dma_map)+{+ struct skb_shared_info *shinfo = xdp_get_shared_info_from_frame(xdpf);+ unsigned long flags;+ int count = 1;+ struct tsnep_tx_entry *entry;+ int length;+ int i;+ int retval;++ if (unlikely(xdp_frame_has_frags(xdpf)))+ count += shinfo->nr_frags;++ spin_lock_irqsave(&tx->lock, flags);++ if (tsnep_tx_desc_available(tx) < (MAX_SKB_FRAGS + 1 + count)) {
Wouldn't count + 1 be sufficient to check against the descs available?
if there are frags then you have already accounted them under count
variable so i feel like MAX_SKB_FRAGS is redundant.
@@ -310,10 +310,11 @@ static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,structtsnep_tx_entry*entry=&tx->entry[index];entry->properties=0;-if(entry->skb){+if(entry->skb||entry->xdpf){
i think this change is redundant, you could keep a single check as skb and
xdpf ptrs share the same memory, but i guess this makes it more obvious
Yes it is actually redundant. I thought it is not a good idea to rely on
the union in the code.
quoted
+/* This function requires __netif_tx_lock is held by the caller. */
+static int tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
+ struct tsnep_tx *tx, bool dma_map)
+{
+ struct skb_shared_info *shinfo = xdp_get_shared_info_from_frame(xdpf);
+ unsigned long flags;
+ int count = 1;
+ struct tsnep_tx_entry *entry;
+ int length;
+ int i;
+ int retval;
+
+ if (unlikely(xdp_frame_has_frags(xdpf)))
+ count += shinfo->nr_frags;
+
+ spin_lock_irqsave(&tx->lock, flags);
+
+ if (tsnep_tx_desc_available(tx) < (MAX_SKB_FRAGS + 1 + count)) {
Wouldn't count + 1 be sufficient to check against the descs available?
if there are frags then you have already accounted them under count
variable so i feel like MAX_SKB_FRAGS is redundant.
In the standard TX path tsnep_xmit_frame_ring() would stop the queue if
less than MAX_SKB_FRAGS + 1 descriptors are available. I wanted to keep
that stop queue logic in tsnep_xmit_frame_ring() by ensuring that XDP
never exceeds this limit (similar to STMMAC_TX_THRESH of stmmac).
So this line checks if enough descriptors are available and that the
queue would not have been stopped by tsnep_xmit_frame_ring().
I could improve the comment below.
quoted
+ /* prevent full TX ring due to XDP */
+ spin_unlock_irqrestore(&tx->lock, flags);
+
+ return -EBUSY;
+ }
this occupies full cacheline, did you make sure that you don't break
tsnep_rx layout with having xdp_rxq_info in the middle of the way?
Actually I did no cacheline optimisation for this structure so far.
I saw that igb/igc put xdp_rxq_info to the end. Is this best practice
to prevent other variables in the same cacheline of xdp_rxq?
From: Gerhard Engleder <hidden> Date: 2022-12-08 22:12:30
On 08.12.22 14:40, Maciej Fijalkowski wrote:
quoted
+static unsigned int tsnep_rx_offset_xdp(void)
+{
+ return XDP_PACKET_HEADROOM;
+}
I don't see much of a value in this func :P
It is a variant of tsnep_rx_offset() for the XDP path to prevent
unneeded calls of tsnep_xdp_is_enabled(). With this function I
keep the RX offset local. But yes, it provides actually no
functionality.
@@ -997,6 +1033,67 @@ static int tsnep_rx_refill(struct tsnep_rx *rx, int count, bool reuse) return i; }+static bool tsnep_xdp_run_prog(struct tsnep_rx *rx, struct bpf_prog *prog,+ struct xdp_buff *xdp, int *status)+{+ unsigned int length;+ unsigned int sync;+ u32 act;++ length = xdp->data_end - xdp->data_hard_start - tsnep_rx_offset_xdp();
could this be xdp->data_end - xdp->data - TSNEP_RX_INLINE_METADATA_SIZE ?
xdp->data points to the start of the Ethernet frame after
TSNEP_RX_INLINE_METADATA_SIZE, so it would be wrong to substract the
metadata which is not there.
Actually xdp->data_end - xdp->data + TSNEP_RX_INLINE_METADATA_SIZE would
be equivalent
TSNEP_RX_INLINE_METADATA_SIZE contains timestamps of received frames. It
is written by DMA at the beginning of the RX buffer. So it extends the
DMA length and needs to be considered for DMA sync.
Can you tell a bit more about that metadata macro that you have to handle
by yourself all the time? would be good to tell about the impact on
data_meta since you're not configuring it on xdp_prepare_buff().
I will add comments.
quoted
+
+ act = bpf_prog_run_xdp(prog, xdp);
+
+ /* Due xdp_adjust_tail: DMA sync for_device cover max len CPU touch */
+ sync = xdp->data_end - xdp->data_hard_start - tsnep_rx_offset_xdp();
+ sync = max(sync, length);
+
+ switch (act) {
+ case XDP_PASS:
+ return false;
+ case XDP_TX:
+ if (tsnep_xdp_xmit_back(rx->adapter, xdp) < 0)
+ goto out_failure;
+ *status |= TSNEP_XDP_TX;
+ return true;
+ case XDP_REDIRECT:
+ if (xdp_do_redirect(rx->adapter->netdev, xdp, prog) < 0)
+ goto out_failure;
+ *status |= TSNEP_XDP_REDIRECT;
+ return true;
+ default:
+ bpf_warn_invalid_xdp_action(rx->adapter->netdev, prog, act);
+ fallthrough;
+ case XDP_ABORTED:
+out_failure:
+ trace_xdp_exception(rx->adapter->netdev, prog, act);
+ fallthrough;
+ case XDP_DROP:
+ page_pool_put_page(rx->page_pool, virt_to_head_page(xdp->data),
+ sync, true);
+ return true;
+ }
+}
+
+static void tsnep_finalize_xdp(struct tsnep_adapter *adapter, int status)
+{
+ int cpu = smp_processor_id();
+ int queue;
+ struct netdev_queue *nq;
do you care about RCT, or?
Do you mean Redundancy Control Trailer (RCT) of PRP? This is new to me.
Do I have to take care about it in the driver? There are no plans to use
redundancy protocols with this device so far.
did you consider making tsnep_build_skb() to work on xdp_buff directly?
probably will help you once you'll implement XDP mbuf support here.
I saw it in other drivers. I did not consider it, because in my opinion
there was no advantage for this driver. Currently xdp_buff is only
initialized on demand if BPF program is there. So for me there was no
reason to change tsnep_build_skb().
quoted
{
@@ -1035,12 +1132,16 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi, int desc_available; int done = 0; enum dma_data_direction dma_dir;+ struct bpf_prog *prog; struct tsnep_rx_entry *entry;+ struct xdp_buff xdp;+ int xdp_status = 0; struct sk_buff *skb; int length; desc_available = tsnep_rx_desc_available(rx); dma_dir = page_pool_get_dma_dir(rx->page_pool);+ prog = READ_ONCE(rx->adapter->xdp_prog); while (likely(done < budget) && (rx->read != rx->write)) { entry = &rx->entry[rx->read];
Implement setup of BPF programs for XDP RX path with command
XDP_SETUP_PROG of ndo_bpf(). This is prework for XDP RX path support.
Signed-off-by: Gerhard Engleder <redacted>
Reserve XDP_PACKET_HEADROOM in front of RX buffer if XDP is enabled.
Also set DMA direction properly in this case.
Signed-off-by: Gerhard Engleder <redacted>
this occupies full cacheline, did you make sure that you don't break
tsnep_rx layout with having xdp_rxq_info in the middle of the way?
Actually I did no cacheline optimisation for this structure so far.
I saw that igb/igc put xdp_rxq_info to the end. Is this best practice
to prevent other variables in the same cacheline of xdp_rxq?
a rule of thumb, organize the structure in the same order they are
being accessed in the data path.. but this doesn't go without saying you
need to do some layout testing via pahole for example..
It's up to you and the maintainer of this driver to decide how critical this
is.
Reviewed-by: Saeed Mahameed <saeed@kernel.org>
return NETDEV_TX_OK;
}
+static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,
+ struct skb_shared_info *shinfo, int count,
+ bool dma_map)
+{
+ struct device *dmadev = tx->adapter->dmadev;
+ skb_frag_t *frag;
+ unsigned int len;
+ struct tsnep_tx_entry *entry;
+ void *data;
+ struct page *page;
+ dma_addr_t dma;
+ int map_len = 0;
+ int i;
+
+ frag = NULL;
+ len = xdpf->len;
+ for (i = 0; i < count; i++) {
+ entry = &tx->entry[(tx->write + i) % TSNEP_RING_SIZE];
+ if (dma_map) {
wouldn't it have made more sense if you passed TSNEP_TX_TYPE instead of
bool dma_map ?
here and in tsnep_xdp_xmit_frame_ring as well..
+ data = unlikely(frag) ? skb_frag_address(frag) :
+ xdpf->data;
+ dma = dma_map_single(dmadev, data, len, DMA_TO_DEVICE);
+ if (dma_mapping_error(dmadev, dma))
+ return -ENOMEM;
+
+ entry->type = TSNEP_TX_TYPE_XDP_NDO;
+ } else {
+ page = unlikely(frag) ? skb_frag_page(frag) :
+ virt_to_page(xdpf->data);
+ dma = page_pool_get_dma_addr(page);
+ if (unlikely(frag))
+ dma += skb_frag_off(frag);
+ else
+ dma += sizeof(*xdpf) + xdpf->headroom;
+ dma_sync_single_for_device(dmadev, dma, len,
+ DMA_BIDIRECTIONAL);
+
+ entry->type = TSNEP_TX_TYPE_XDP_TX;
+ }
+
+ entry->len = len;
+ dma_unmap_addr_set(entry, dma, dma);
+
+ entry->desc->tx = __cpu_to_le64(dma);
+
+ map_len += len;
+
+ if ((i + 1) < count) {
+ frag = &shinfo->frags[i];
+ len = skb_frag_size(frag);
+ }
+ }
+
+ return map_len;
+}
+
+/* This function requires __netif_tx_lock is held by the caller. */
+static int tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
+ struct tsnep_tx *tx, bool dma_map)
+{
+ struct skb_shared_info *shinfo = xdp_get_shared_info_from_frame(xdpf);
+ unsigned long flags;
+ int count = 1;
+ struct tsnep_tx_entry *entry;
+ int length;
+ int i;
+ int retval;
Maciiej already commented on this, and i agree with him, the whole series
needs some work on rev xmas tree variable declaration, code will look much
neater.
+
+ if (unlikely(xdp_frame_has_frags(xdpf)))
+ count += shinfo->nr_frags;
+
+ spin_lock_irqsave(&tx->lock, flags);
+
+ if (tsnep_tx_desc_available(tx) < (MAX_SKB_FRAGS + 1 + count)) {
+ /* prevent full TX ring due to XDP */
+ spin_unlock_irqrestore(&tx->lock, flags);
+
+ return -EBUSY;
You don't really do anything with the retval, so just return a boolean.
From: Gerhard Engleder <hidden> Date: 2022-12-09 08:02:17
On 09.12.22 03:23, Saeed Mahameed wrote:
quoted
+static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,
+ struct skb_shared_info *shinfo, int count,
+ bool dma_map)
+{
+ struct device *dmadev = tx->adapter->dmadev;
+ skb_frag_t *frag;
+ unsigned int len;
+ struct tsnep_tx_entry *entry;
+ void *data;
+ struct page *page;
+ dma_addr_t dma;
+ int map_len = 0;
+ int i;
+
+ frag = NULL;
+ len = xdpf->len;
+ for (i = 0; i < count; i++) {
+ entry = &tx->entry[(tx->write + i) % TSNEP_RING_SIZE];
+ if (dma_map) {
wouldn't it have made more sense if you passed TSNEP_TX_TYPE instead of
bool dma_map ?
here and in tsnep_xdp_xmit_frame_ring as well..
I will give it a try.
quoted
+/* This function requires __netif_tx_lock is held by the caller. */
+static int tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
+ struct tsnep_tx *tx, bool dma_map)
+{
+ struct skb_shared_info *shinfo =
xdp_get_shared_info_from_frame(xdpf);
+ unsigned long flags;
+ int count = 1;
+ struct tsnep_tx_entry *entry;
+ int length;
+ int i;
+ int retval;
Maciiej already commented on this, and i agree with him, the whole series
needs some work on rev xmas tree variable declaration, code will look much
neater.
So far I ordered the variable declaration by variable usage with common
variables like i and retval at the end. I will take a look an that.
quoted
+
+ if (unlikely(xdp_frame_has_frags(xdpf)))
+ count += shinfo->nr_frags;
+
+ spin_lock_irqsave(&tx->lock, flags);
+
+ if (tsnep_tx_desc_available(tx) < (MAX_SKB_FRAGS + 1 + count)) {
+ /* prevent full TX ring due to XDP */
+ spin_unlock_irqrestore(&tx->lock, flags);
+
+ return -EBUSY;
You don't really do anything with the retval, so just return a boolean.
this occupies full cacheline, did you make sure that you don't break
tsnep_rx layout with having xdp_rxq_info in the middle of the way?
Actually I did no cacheline optimisation for this structure so far.
I saw that igb/igc put xdp_rxq_info to the end. Is this best practice
to prevent other variables in the same cacheline of xdp_rxq?
a rule of thumb, organize the structure in the same order they are
being accessed in the data path.. but this doesn't go without saying you
need to do some layout testing via pahole for example..
It's up to you and the maintainer of this driver to decide how critical
this
is.
Reviewed-by: Saeed Mahameed <saeed@kernel.org>
Thanks for the clarification, I will think about it.
I wrote the driver and I'm responsible to keep it working. Is this equal
to being the maintainer?
Thanks for the review!
Gerhard
this could fail silently, and then cause double free, when close ndo
will be called, the stack won't be aware of the closed state..
I will ensure that no double free will happen when ndo_close is called.
Other drivers like igc/igb/netsec/stmmac also fail silently and
I don't see any measures against double free in this drivers.
mvneta forwards the return value of open. How are these drivers
solving this issue? I cannot find this detail, but I also do not
believe that all this drivers are buggy.
gerhard