Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Amazon ENA driver #264

Open
c4milo opened this issue Dec 20, 2016 · 9 comments
Open

Support for Amazon ENA driver #264

c4milo opened this issue Dec 20, 2016 · 9 comments

Comments

@c4milo
Copy link

c4milo commented Dec 20, 2016

It would be great if netmap had support for this driver, it only ships with DPDK support.

https://aws.amazon.com/blogs/aws/elastic-network-adapter-high-performance-network-interface-for-amazon-ec2/

@vmaffione
Copy link
Collaborator

Indeed, it would be great, but we don't have the ENA hardware to play with, nor ENA specifications.
In general, netmap support requires ~600 lines of code to be written, which is a very limited amount.
We can guide you through the process, if you wish.

@username-x
Copy link

username-x commented Dec 28, 2016

Hi!
I'm very interested in guidelines for developing netmap-friendly drivers. I have network adapter based on FPGA. It would be so great if you cloud give some tips.

@vmaffione
Copy link
Collaborator

vmaffione commented Dec 29, 2016 via email

@gspivey
Copy link
Contributor

gspivey commented Apr 16, 2017

@vmaffione
Amazon has been posting the source for the ena drivers to github.
https://github.com/amzn/amzn-drivers

DPDK seems to already support ena.
http://dpdk.org/doc/guides/nics/ena.html

I am willing to give this a shot if you don't mind giving feedback.

@vmaffione
Copy link
Collaborator

Hi Gerard,
I've taken a quick look to the ena linux driver, it seems to follow the usual Intel NIC driver structure (see e1000, ixgbe, igb, etc.). This is not surprising as people willing to write a new Linux NIC driver take inspiration from Intel drivers that are well written and stable.

Anyway I see the datapath operation is sketched quite clearly here
https://github.com/amzn/amzn-drivers/blob/master/kernel/linux/ena/README

Netmap support in principle requires way less effort than DPDK support, as only a small datapath-related patch is needed. The original driver is still in charge of everything related to configuration and control path (apart from management of RX buffer refill, see above).

If you want to try sketching a netmap patch for ena we can give you feedback and suggestions for sure.
You can follow the guidelines above.

@gspivey
Copy link
Contributor

gspivey commented May 9, 2017

Hello @vmaffione,

Thanks for your support. I have read through your guidelines, netmap source, and ena source in more detail. I will attempt to implement this in five phases (you list two phase 4's) as you described in your guidelines.

I will post a gist with a prototype of steps 1a-1c.
It will include a function ena_netmap_attach
And three patches of ena_netdev.c for:

  1. ena_netmap_attach
  2. netmap_detach
  3. netmap_rx_irq

Questions:
Is netmap_init_buffers optional (Step5)? i40e_netmap_linux.h does not seem to implement it.

@vmaffione
Copy link
Collaborator

Hi Gerard,
My bad on the two phase 4's, I've fixed the guidelines.

Your plan looks great to me.

Regarding the netmap_init_buffer(), it is not optional (nothing it is optional in the list).
In i40e, this functionality is implemented by i40e_netmap_configure_rx_ring(). The function names are just chosen to be consistent with the original driver. This step is necessary to let the NIC DMA received frames into netmap memory rather than memory attached to sk_buffs.

@gspivey
Copy link
Contributor

gspivey commented May 11, 2017

Hey @vmaffione
Mind taking a look at my notes below?
If they look good I will move on to phase 2.

Thanks again for your support.

Gerard

ENA Netmap Prototype Phase 1

Steps for phase one are as follows:

ena_netmap_linux.h prototype

#include <bsd_glue.h>
#include <net/netmap.h>
#include <netmap/netmap_kern.h>

#ifdef NETMAP_LINUX_ENA_PTR_ARRAY
#define NM_ENA_TX_RING(a, r)           ((a)->tx_rings[(r)])
#define NM_ENA_RX_RING(a, r)           ((a)->rx_rings[(r)])
#else
#define NM_ENA_TX_RING(a, r)           (&(a)->tx_rings[(r)])
#define NM_ENA_RX_RING(a, r)           (&(a)->rx_rings[(r)])
#endif
/*
 * The attach routine, called near the end of ena_probe(),
 * fills the parameters for netmap_attach() and calls it.
 * It cannot fail, in the worst case (such as no memory)
 * netmap mode will be disabled and the driver will only
 * operate in standard mode.
 */
static void
ena_netmap_attach(struct ena_adapter *adapter)
{
        struct netmap_adapter na;

        bzero(&na, sizeof(na));

        na.ifp = adapter->netdev;
        na.na_flags = NAF_BDG_MAYSLEEP;
        na.pdev = &adapter->pdev->dev;
        // XXX check that queues is set.
        na.num_tx_desc = NM_ENA_TX_RING(adapter, 0)->count;
        na.num_rx_desc = NM_ENA_RX_RING(adapter, 0)->count;
        // na.nm_txsync = ena_netmap_txsync; // Task 3
        // na.nm_rxsync = ena_netmap_rxsync; // Task 4
        // na.nm_register = ena_netmap_reg; // Task 2
        na.num_tx_rings = na.num_rx_rings = adapter->num_queue_pairs;

        netmap_attach(&na);
}

ena_netmap.c patch

Patch is to 1.1.3 tag of ena_netdev.c

diff --git a/ena_netdev.c b/ena_netdev.c
index 0facf46..445857b 100644
--- a/ena_netdev.c
+++ b/ena_netdev.c
@@ -54,6 +54,10 @@
 #include "ena_pci_id_tbl.h"
 #include "ena_sysfs.h"
 
+#if defined(CONFIG_NETMAP) || defined(CONFIG_NETMAP_MODULE)
+#include <ena_netmap.h>
+#endif
+
 static char version[] = DEVICE_NAME " v" DRV_MODULE_VERSION "\n";
 
 MODULE_AUTHOR("Amazon.com, Inc. or its affiliates");
@@ -696,6 +700,13 @@ static int ena_clean_tx_irq(struct ena_ring *tx_ring, u32 budget)
        int tx_pkts = 0;
        int rc;
 
+       struct ena_adapter *adapter = tx_ring->adapter;
+       struct net_device *netdev = adapter->netdev;
+#ifdef DEV_NETMAP
+        if (netmap_tx_irq(netdev, 0))
+                return true; /* cleaned ok */
+#endif /* DEV_NETMAP */
+
        next_to_clean = tx_ring->next_to_clean;
        txq = netdev_get_tx_queue(tx_ring->netdev, tx_ring->qid);
 
@@ -1013,6 +1024,18 @@ static int ena_clean_rx_irq(struct ena_ring *rx_ring, struct napi_struct *napi,
        int total_len = 0;
        int rx_copybreak_pkt = 0;
 
+        struct net_device *netdev = adapter->netdev;
+#ifdef DEV_NETMAP
+#ifdef CONFIG_ENA_NAPI
+#define NETMAP_DUMMY work_done
+#else
+        int dummy;
+#define NETMAP_DUMMY &dummy
+#endif
+        if (netmap_rx_irq(netdev, 0, NETMAP_DUMMY))
+                return true;
+#endif /* DEV_NETMAP */
+
        netif_dbg(rx_ring->adapter, rx_status, rx_ring->netdev,
                  "%s qid %d\n", __func__, rx_ring->qid);
        res_budget = budget;
@@ -3377,6 +3400,10 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
        adapter->timer_service.function = ena_timer_service;
        adapter->timer_service.data = (unsigned long)adapter;
 
+#ifdef DEV_NETMAP
+        ena_netmap_attach(adapter);
+#endif /* DEV_NETMAP */
+
        add_timer(&adapter->timer_service);
 
        dev_info(&pdev->dev, "%s found at mem %lx, mac addr %pM Queues %d\n",
@@ -3517,6 +3544,11 @@ static void ena_remove(struct pci_dev *pdev)
        ena_com_destroy_interrupt_moderation(ena_dev);
 
        vfree(ena_dev);
+
+#ifdef DEV_NETMAP
+        netmap_detach(netdev);
+#endif /* DEV_NETMAP */
+
 }
 
 static struct pci_driver ena_pci_driver = {

@vmaffione
Copy link
Collaborator

Hi @gspivey .
It looks quite ok, but I have some comments, mostly related to the fact that you should have taken ixgbe as a reference more than i40e (there are some small things to be fixed in i40e, while ixgbe support is more mature).

  • Why did you add the NETMAP_LINUX_ENA_PTR_ARRAY macros? Those are specific to i40e, to manage different driver versions. I see in the ena driver you can just use adapter->tx_ring[N] or adapter->rx_ring[N].
  • Remove the line setting na_flags NAF_BDG_MAYSLEEP. This can be set on FreeBSD drivers, but not on Linux ones. That's our fault, we will remove this line also from i40e.
  • The logic after the netmap_rx_irq() call should be a bit more complex, see for instance LINUX/final-patches/intel--ixgbe--5.0.4 .
  • I think you have an inconsistency in the include directive: ena_netmap.h vs ena_netmap_linux.h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants