lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CALDO+SagXG8n_3nGiPdoae=-Bx0NJY9cXp8RJ4jZ9ShN6bxRnQ@mail.gmail.com> Date: Tue, 10 Jan 2023 06:23:12 -0800 From: William Tu <u9012063@...il.com> To: Alexander H Duyck <alexander.duyck@...il.com> Cc: netdev@...r.kernel.org, tuc@...are.com, gyang@...are.com, doshir@...are.com, gerhard@...leder-embedded.com, alexandr.lobakin@...el.com, bang@...are.com Subject: Re: [RFC PATCH net-next v11] vmxnet3: Add XDP support. Hi Alexander, Thanks for your review! On Mon, Jan 9, 2023 at 11:02 AM Alexander H Duyck <alexander.duyck@...il.com> wrote: > > On Sun, 2023-01-08 at 10:18 -0800, William Tu wrote: > > The patch adds native-mode XDP support: XDP DROP, PASS, TX, and REDIRECT. > > <...> > > char vmxnet3_driver_name[] = "vmxnet3"; > > #define VMXNET3_DRIVER_DESC "VMware vmxnet3 virtual NIC driver" > > @@ -322,44 +323,58 @@ static u32 get_bitfield32(const __le32 *bitfield, u32 pos, u32 size) > > #endif /* __BIG_ENDIAN_BITFIELD */ > > > > > > -static void > > -vmxnet3_unmap_tx_buf(struct vmxnet3_tx_buf_info *tbi, > > - struct pci_dev *pdev) > > +static u32 > > +vmxnet3_unmap_tx_buf(struct vmxnet3_tx_buf_info *tbi, struct pci_dev *pdev, > > + struct xdp_frame_bulk *bq) > > Is the bq value being used anywhere in here? We probably need to either > drop it. yes, my mistake. > > > { > > - if (tbi->map_type == VMXNET3_MAP_SINGLE) > > + u32 map_type = tbi->map_type; > > + > > + if (map_type & VMXNET3_MAP_SINGLE) > > dma_unmap_single(&pdev->dev, tbi->dma_addr, tbi->len, > > DMA_TO_DEVICE); > > - else if (tbi->map_type == VMXNET3_MAP_PAGE) > > + else if (map_type & VMXNET3_MAP_PAGE) > > dma_unmap_page(&pdev->dev, tbi->dma_addr, tbi->len, > > DMA_TO_DEVICE); > > - else > > - BUG_ON(tbi->map_type != VMXNET3_MAP_NONE); > > + else if (map_type & ~(VMXNET3_MAP_SINGLE | VMXNET3_MAP_PAGE | > > + VMXNET3_MAP_XDP)) > > + BUG_ON(map_type != VMXNET3_MAP_NONE); > > What you might just do here is drop the "else if" in favor of an > "else". Your bug on would basically just need to check for: > else > BUG_ON((map_type & ~VMXNET_MAP_XDP)); Will do it, thanks! > > > tbi->map_type = VMXNET3_MAP_NONE; /* to help debugging */ > > + > > + return map_type; > > } > > > > > > In reality we only need to worry about the map_type of the eop buffer > when we are dealing with any frame. You might actually pull the > map_type out in vmxnet3_unmap_pkt below and just pass that to the end. > > > static int > > vmxnet3_unmap_pkt(u32 eop_idx, struct vmxnet3_tx_queue *tq, > > - struct pci_dev *pdev, struct vmxnet3_adapter *adapter) > > + struct pci_dev *pdev, struct vmxnet3_adapter *adapter, > > + struct xdp_frame_bulk *bq) > > { > > - struct sk_buff *skb; > > + struct vmxnet3_tx_buf_info *tbi; > > int entries = 0; > > + u32 map_type; > > > > /* no out of order completion */ > > BUG_ON(tq->buf_info[eop_idx].sop_idx != tq->tx_ring.next2comp); > > BUG_ON(VMXNET3_TXDESC_GET_EOP(&(tq->tx_ring.base[eop_idx].txd)) != 1); > > > > - skb = tq->buf_info[eop_idx].skb; > > - BUG_ON(skb == NULL); > > - tq->buf_info[eop_idx].skb = NULL; > > - > > + tbi = &tq->buf_info[eop_idx]; > > VMXNET3_INC_RING_IDX_ONLY(eop_idx, tq->tx_ring.size); > > > > We may want to record the map_type here for the tbi that contains the > skb or XDP frame. Then we don't need to record the map_type in the loop > below and can save ourselves a few cycles. > > Also we may want to preserve the BUG_ON check, but here it would be: > BUG_ON(tbi->skb == NULL); Got it, thanks! > > > while (tq->tx_ring.next2comp != eop_idx) { > > - vmxnet3_unmap_tx_buf(tq->buf_info + tq->tx_ring.next2comp, > > - pdev); > > - > > + map_type = vmxnet3_unmap_tx_buf(tq->buf_info + > > + tq->tx_ring.next2comp, pdev, > > + bq); > > + /* xdpf and skb are in an anonymous union, if set we need to > > + * free a buffer. > > + */ > > + if (tbi->skb) { > > + if (map_type & VMXNET3_MAP_XDP) > > + xdp_return_frame_bulk(tbi->xdpf, bq); > > + else > > + dev_kfree_skb_any(tbi->skb); > > + tbi->skb = NULL; > > + } > > So this code could probably be moved to the end of the function where > the dev_kfree_skb_any originally was. Then you can drop the "if(tbi- > >skb)" and instead just have the map_type dealt with there. This should > save you from having to read and store map_type multiple times. > > > /* update next2comp w/o tx_lock. Since we are marking more, > > * instead of less, tx ring entries avail, the worst case is > > * that the tx routine incorrectly re-queues a pkt due to > > @@ -369,7 +384,6 @@ vmxnet3_unmap_pkt(u32 eop_idx, struct vmxnet3_tx_queue *tq, > > entries++; > > } > > > > - dev_kfree_skb_any(skb); > > return entries; > > } > > > > No point moving this into the loop when it can always be processed at > the end. Basically we just needc to pull the if statement you added > above down to here. right, will move out of the loop. > > > @@ -379,8 +393,10 @@ vmxnet3_tq_tx_complete(struct vmxnet3_tx_queue *tq, > > struct vmxnet3_adapter *adapter) > > { > > int completed = 0; > > + struct xdp_frame_bulk bq; > > union Vmxnet3_GenericDesc *gdesc; > > > > + xdp_frame_bulk_init(&bq); > > gdesc = tq->comp_ring.base + tq->comp_ring.next2proc; > > while (VMXNET3_TCD_GET_GEN(&gdesc->tcd) == tq->comp_ring.gen) { > > /* Prevent any &gdesc->tcd field from being (speculatively) > > @@ -390,11 +406,12 @@ vmxnet3_tq_tx_complete(struct vmxnet3_tx_queue *tq, > > > > completed += vmxnet3_unmap_pkt(VMXNET3_TCD_GET_TXIDX( > > &gdesc->tcd), tq, adapter->pdev, > > - adapter); > > + adapter, &bq); > > > > vmxnet3_comp_ring_adv_next2proc(&tq->comp_ring); > > gdesc = tq->comp_ring.base + tq->comp_ring.next2proc; > > } > > + xdp_flush_frame_bulk(&bq); > > > > if (completed) { > > spin_lock(&tq->tx_lock); > > @@ -414,26 +431,33 @@ static void > > vmxnet3_tq_cleanup(struct vmxnet3_tx_queue *tq, > > struct vmxnet3_adapter *adapter) > > { > > + struct xdp_frame_bulk bq; > > + u32 map_type; > > int i; > > > > + xdp_frame_bulk_init(&bq); > > + > > while (tq->tx_ring.next2comp != tq->tx_ring.next2fill) { > > struct vmxnet3_tx_buf_info *tbi; > > > > tbi = tq->buf_info + tq->tx_ring.next2comp; > > > > Rather than have vmxnet3_unmap_tx_buf return the map_type you might > just read it yourself here. Will do it, thanks William
Powered by blists - more mailing lists