Re: [net-next PATCH v2 1/2] e1000: add initial XDP support

[John Fastabend]

On 16-09-20 09:26 PM, zhuyj wrote:
>  +static int e1000_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
> +{
> +   struct e1000_adapter *adapter = netdev_priv(netdev);
> +   struct bpf_prog *old_prog;
> +
> +   old_prog = xchg(>prog, prog);
> +   if (old_prog) {
> +   synchronize_net();
> +   bpf_prog_put(old_prog);
> +   }
> +
> +   if (netif_running(netdev))
> +   e1000_reinit_locked(adapter);
> +   else
> +   e1000_reset(adapter);
> +   return 0;
> +}
> 
> To this function, is it better to use "static void
> e1000_xdp_set(struct net_device *netdev, struct bpf_prog *prog)"?
> since it is always to return 0.
> 

In general try to avoid top posting.

Yes making it void would be reasonable and probably a good idea. I'll
do it in v3.

[...]

Thanks,
John

Re: [net-next PATCH v2 1/2] e1000: add initial XDP support

[zhuyj]

 +static int e1000_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
+{
+   struct e1000_adapter *adapter = netdev_priv(netdev);
+   struct bpf_prog *old_prog;
+
+   old_prog = xchg(>prog, prog);
+   if (old_prog) {
+   synchronize_net();
+   bpf_prog_put(old_prog);
+   }
+
+   if (netif_running(netdev))
+   e1000_reinit_locked(adapter);
+   else
+   e1000_reset(adapter);
+   return 0;
+}

To this function, is it better to use "static void
e1000_xdp_set(struct net_device *netdev, struct bpf_prog *prog)"?
since it is always to return 0.


On Sat, Sep 10, 2016 at 5:29 AM, John Fastabend
 wrote:
> From: Alexei Starovoitov 
>
> This patch adds initial support for XDP on e1000 driver. Note e1000
> driver does not support page recycling in general which could be
> added as a further improvement. However XDP_DROP case will recycle.
> XDP_TX and XDP_PASS do not support recycling yet.
>
> I tested this patch running e1000 in a VM using KVM over a tap
> device.
>
> CC: William Tu 
> Signed-off-by: Alexei Starovoitov 
> Signed-off-by: John Fastabend 
> ---
>  drivers/net/ethernet/intel/e1000/e1000.h  |2
>  drivers/net/ethernet/intel/e1000/e1000_main.c |  171 
> +
>  2 files changed, 170 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000/e1000.h 
> b/drivers/net/ethernet/intel/e1000/e1000.h
> index d7bdea7..5cf8a0a 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000.h
> +++ b/drivers/net/ethernet/intel/e1000/e1000.h
> @@ -150,6 +150,7 @@ struct e1000_adapter;
>   */
>  struct e1000_tx_buffer {
> struct sk_buff *skb;
> +   struct page *page;
> dma_addr_t dma;
> unsigned long time_stamp;
> u16 length;
> @@ -279,6 +280,7 @@ struct e1000_adapter {
>  struct e1000_rx_ring *rx_ring,
>  int cleaned_count);
> struct e1000_rx_ring *rx_ring;  /* One per active queue */
> +   struct bpf_prog *prog;
> struct napi_struct napi;
>
> int num_tx_queues;
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c 
> b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index f42129d..91d5c87 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -32,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  char e1000_driver_name[] = "e1000";
>  static char e1000_driver_string[] = "Intel(R) PRO/1000 Network Driver";
> @@ -842,6 +843,44 @@ static int e1000_set_features(struct net_device *netdev,
> return 0;
>  }
>
> +static int e1000_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
> +{
> +   struct e1000_adapter *adapter = netdev_priv(netdev);
> +   struct bpf_prog *old_prog;
> +
> +   old_prog = xchg(>prog, prog);
> +   if (old_prog) {
> +   synchronize_net();
> +   bpf_prog_put(old_prog);
> +   }
> +
> +   if (netif_running(netdev))
> +   e1000_reinit_locked(adapter);
> +   else
> +   e1000_reset(adapter);
> +   return 0;
> +}
> +
> +static bool e1000_xdp_attached(struct net_device *dev)
> +{
> +   struct e1000_adapter *priv = netdev_priv(dev);
> +
> +   return !!priv->prog;
> +}
> +
> +static int e1000_xdp(struct net_device *dev, struct netdev_xdp *xdp)
> +{
> +   switch (xdp->command) {
> +   case XDP_SETUP_PROG:
> +   return e1000_xdp_set(dev, xdp->prog);
> +   case XDP_QUERY_PROG:
> +   xdp->prog_attached = e1000_xdp_attached(dev);
> +   return 0;
> +   default:
> +   return -EINVAL;
> +   }
> +}
> +
>  static const struct net_device_ops e1000_netdev_ops = {
> .ndo_open   = e1000_open,
> .ndo_stop   = e1000_close,
> @@ -860,6 +899,7 @@ static const struct net_device_ops e1000_netdev_ops = {
>  #endif
> .ndo_fix_features   = e1000_fix_features,
> .ndo_set_features   = e1000_set_features,
> +   .ndo_xdp= e1000_xdp,
>  };
>
>  /**
> @@ -1276,6 +1316,9 @@ static void e1000_remove(struct pci_dev *pdev)
> e1000_down_and_stop(adapter);
> e1000_release_manageability(adapter);
>
> +   if (adapter->prog)
> +   bpf_prog_put(adapter->prog);
> +
> unregister_netdev(netdev);
>
> e1000_phy_hw_reset(hw);
> @@ -1859,7 +1902,7 @@ static void e1000_configure_rx(struct e1000_adapter 
> *adapter)
> struct e1000_hw *hw = >hw;
> u32 rdlen, rctl, rxcsum;
>
> -   if (adapter->netdev->mtu > ETH_DATA_LEN) {
> +   if (adapter->netdev->mtu > ETH_DATA_LEN || adapter->prog) {
> rdlen = adapter->rx_ring[0].count *
> sizeof(struct e1000_rx_desc);
>   

Re: [net-next PATCH v2 1/2] e1000: add initial XDP support

[John Fastabend]

On 16-09-09 03:04 PM, Eric Dumazet wrote:
> On Fri, 2016-09-09 at 14:29 -0700, John Fastabend wrote:
>> From: Alexei Starovoitov 
>>
> 
> 
> So it looks like e1000_xmit_raw_frame() can return early,
> say if there is no available descriptor.
> 
>> +static void e1000_xmit_raw_frame(struct e1000_rx_buffer *rx_buffer_info,
>> + unsigned int len,
>> + struct net_device *netdev,
>> + struct e1000_adapter *adapter)
>> +{
>> +struct netdev_queue *txq = netdev_get_tx_queue(netdev, 0);
>> +struct e1000_hw *hw = >hw;
>> +struct e1000_tx_ring *tx_ring;
>> +
>> +if (len > E1000_MAX_DATA_PER_TXD)
>> +return;
>> +
>> +/* e1000 only support a single txq at the moment so the queue is being
>> + * shared with stack. To support this requires locking to ensure the
>> + * stack and XDP are not running at the same time. Devices with
>> + * multiple queues should allocate a separate queue space.
>> + */
>> +HARD_TX_LOCK(netdev, txq, smp_processor_id());
>> +
>> +tx_ring = adapter->tx_ring;
>> +
>> +if (E1000_DESC_UNUSED(tx_ring) < 2) {
>> +HARD_TX_UNLOCK(netdev, txq);
>> +return;
>> +}
>> +
>> +e1000_tx_map_rxpage(tx_ring, rx_buffer_info, len);
>> +e1000_tx_queue(adapter, tx_ring, 0/*tx_flags*/, 1);
>> +
>> +writel(tx_ring->next_to_use, hw->hw_addr + tx_ring->tdt);
>> +mmiowb();
>> +
>> +HARD_TX_UNLOCK(netdev, txq);
>> +}
>> +
>>  #define NUM_REGS 38 /* 1 based count */
>>  static void e1000_regdump(struct e1000_adapter *adapter)
>>  {
>> @@ -4142,6 +4247,19 @@ static struct sk_buff *e1000_alloc_rx_skb(struct 
>> e1000_adapter *adapter,
>>  return skb;
>>  }
>> +act = e1000_call_bpf(prog, page_address(p), length);
>> +switch (act) {
>> +case XDP_PASS:
>> +break;
>> +case XDP_TX:
>> +dma_sync_single_for_device(>dev,
>> +   dma,
>> +   length,
>> +   DMA_TO_DEVICE);
>> +e1000_xmit_raw_frame(buffer_info, length,
>> + netdev, adapter);
>> +buffer_info->rxbuf.page = NULL;
> 
> 
> So I am trying to understand how pages are not leaked ?
> 
> 

Pages are being leaked thanks! v3 coming soon.

Re: [net-next PATCH v2 1/2] e1000: add initial XDP support

[Eric Dumazet]

On Fri, 2016-09-09 at 14:29 -0700, John Fastabend wrote:
> From: Alexei Starovoitov 
> 


So it looks like e1000_xmit_raw_frame() can return early,
say if there is no available descriptor.

> +static void e1000_xmit_raw_frame(struct e1000_rx_buffer *rx_buffer_info,
> +  unsigned int len,
> +  struct net_device *netdev,
> +  struct e1000_adapter *adapter)
> +{
> + struct netdev_queue *txq = netdev_get_tx_queue(netdev, 0);
> + struct e1000_hw *hw = >hw;
> + struct e1000_tx_ring *tx_ring;
> +
> + if (len > E1000_MAX_DATA_PER_TXD)
> + return;
> +
> + /* e1000 only support a single txq at the moment so the queue is being
> +  * shared with stack. To support this requires locking to ensure the
> +  * stack and XDP are not running at the same time. Devices with
> +  * multiple queues should allocate a separate queue space.
> +  */
> + HARD_TX_LOCK(netdev, txq, smp_processor_id());
> +
> + tx_ring = adapter->tx_ring;
> +
> + if (E1000_DESC_UNUSED(tx_ring) < 2) {
> + HARD_TX_UNLOCK(netdev, txq);
> + return;
> + }
> +
> + e1000_tx_map_rxpage(tx_ring, rx_buffer_info, len);
> + e1000_tx_queue(adapter, tx_ring, 0/*tx_flags*/, 1);
> +
> + writel(tx_ring->next_to_use, hw->hw_addr + tx_ring->tdt);
> + mmiowb();
> +
> + HARD_TX_UNLOCK(netdev, txq);
> +}
> +
>  #define NUM_REGS 38 /* 1 based count */
>  static void e1000_regdump(struct e1000_adapter *adapter)
>  {
> @@ -4142,6 +4247,19 @@ static struct sk_buff *e1000_alloc_rx_skb(struct 
> e1000_adapter *adapter,
>   return skb;
>  }
> + act = e1000_call_bpf(prog, page_address(p), length);
> + switch (act) {
> + case XDP_PASS:
> + break;
> + case XDP_TX:
> + dma_sync_single_for_device(>dev,
> +dma,
> +length,
> +DMA_TO_DEVICE);
> + e1000_xmit_raw_frame(buffer_info, length,
> +  netdev, adapter);
> + buffer_info->rxbuf.page = NULL;


So I am trying to understand how pages are not leaked ?

[net-next PATCH v2 1/2] e1000: add initial XDP support

[John Fastabend]

From: Alexei Starovoitov 

This patch adds initial support for XDP on e1000 driver. Note e1000
driver does not support page recycling in general which could be
added as a further improvement. However XDP_DROP case will recycle.
XDP_TX and XDP_PASS do not support recycling yet.

I tested this patch running e1000 in a VM using KVM over a tap
device.

CC: William Tu 
Signed-off-by: Alexei Starovoitov 
Signed-off-by: John Fastabend 
---
 drivers/net/ethernet/intel/e1000/e1000.h  |2 
 drivers/net/ethernet/intel/e1000/e1000_main.c |  171 +
 2 files changed, 170 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000.h 
b/drivers/net/ethernet/intel/e1000/e1000.h
index d7bdea7..5cf8a0a 100644
--- a/drivers/net/ethernet/intel/e1000/e1000.h
+++ b/drivers/net/ethernet/intel/e1000/e1000.h
@@ -150,6 +150,7 @@ struct e1000_adapter;
  */
 struct e1000_tx_buffer {
struct sk_buff *skb;
+   struct page *page;
dma_addr_t dma;
unsigned long time_stamp;
u16 length;
@@ -279,6 +280,7 @@ struct e1000_adapter {
 struct e1000_rx_ring *rx_ring,
 int cleaned_count);
struct e1000_rx_ring *rx_ring;  /* One per active queue */
+   struct bpf_prog *prog;
struct napi_struct napi;
 
int num_tx_queues;
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c 
b/drivers/net/ethernet/intel/e1000/e1000_main.c
index f42129d..91d5c87 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 
 char e1000_driver_name[] = "e1000";
 static char e1000_driver_string[] = "Intel(R) PRO/1000 Network Driver";
@@ -842,6 +843,44 @@ static int e1000_set_features(struct net_device *netdev,
return 0;
 }
 
+static int e1000_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
+{
+   struct e1000_adapter *adapter = netdev_priv(netdev);
+   struct bpf_prog *old_prog;
+
+   old_prog = xchg(>prog, prog);
+   if (old_prog) {
+   synchronize_net();
+   bpf_prog_put(old_prog);
+   }
+
+   if (netif_running(netdev))
+   e1000_reinit_locked(adapter);
+   else
+   e1000_reset(adapter);
+   return 0;
+}
+
+static bool e1000_xdp_attached(struct net_device *dev)
+{
+   struct e1000_adapter *priv = netdev_priv(dev);
+
+   return !!priv->prog;
+}
+
+static int e1000_xdp(struct net_device *dev, struct netdev_xdp *xdp)
+{
+   switch (xdp->command) {
+   case XDP_SETUP_PROG:
+   return e1000_xdp_set(dev, xdp->prog);
+   case XDP_QUERY_PROG:
+   xdp->prog_attached = e1000_xdp_attached(dev);
+   return 0;
+   default:
+   return -EINVAL;
+   }
+}
+
 static const struct net_device_ops e1000_netdev_ops = {
.ndo_open   = e1000_open,
.ndo_stop   = e1000_close,
@@ -860,6 +899,7 @@ static const struct net_device_ops e1000_netdev_ops = {
 #endif
.ndo_fix_features   = e1000_fix_features,
.ndo_set_features   = e1000_set_features,
+   .ndo_xdp= e1000_xdp,
 };
 
 /**
@@ -1276,6 +1316,9 @@ static void e1000_remove(struct pci_dev *pdev)
e1000_down_and_stop(adapter);
e1000_release_manageability(adapter);
 
+   if (adapter->prog)
+   bpf_prog_put(adapter->prog);
+
unregister_netdev(netdev);
 
e1000_phy_hw_reset(hw);
@@ -1859,7 +1902,7 @@ static void e1000_configure_rx(struct e1000_adapter 
*adapter)
struct e1000_hw *hw = >hw;
u32 rdlen, rctl, rxcsum;
 
-   if (adapter->netdev->mtu > ETH_DATA_LEN) {
+   if (adapter->netdev->mtu > ETH_DATA_LEN || adapter->prog) {
rdlen = adapter->rx_ring[0].count *
sizeof(struct e1000_rx_desc);
adapter->clean_rx = e1000_clean_jumbo_rx_irq;
@@ -1973,6 +2016,11 @@ e1000_unmap_and_free_tx_resource(struct e1000_adapter 
*adapter,
dev_kfree_skb_any(buffer_info->skb);
buffer_info->skb = NULL;
}
+   if (buffer_info->page) {
+   put_page(buffer_info->page);
+   buffer_info->page = NULL;
+   }
+
buffer_info->time_stamp = 0;
/* buffer_info must be completely set up in the transmit path */
 }
@@ -3298,6 +3346,63 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
return NETDEV_TX_OK;
 }
 
+static void e1000_tx_map_rxpage(struct e1000_tx_ring *tx_ring,
+   struct e1000_rx_buffer *rx_buffer_info,
+   unsigned int len)
+{
+   struct e1000_tx_buffer *buffer_info;
+   unsigned int i = tx_ring->next_to_use;
+
+   buffer_info = _ring->buffer_info[i];
+
+