[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [UNIKRAFT PATCH v4 06/12] plat/xen/drivers/net: Configure netfront tx queue
On 23.10.20 17:18, Costin Lupu wrote:
Hi Sharan,
Please see inline.
On 10/22/20 2:53 PM, Sharan Santhanam wrote:
Hello Costin,
Please find the comments inline:
Thanks & Regards
Sharan
On 8/13/20 10:53 AM, Costin Lupu wrote:
For each queue, either rx or tx, the packets references are kept in a Xen
shared ring. This patch introduces the initialization of tx shared
ring. Each
time the driver will send a packet it will add a request on the tx
shared ring
which will be serviced by the backend. Therefore we need to keep track
of the
requests by keeping track of their IDs. Because our resources are
limited, we
will maintain a pool of IDs (see `freelist` array) for this purpose.
Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
Signed-off-by: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
---
plat/xen/drivers/net/netfront.c | 108 ++++++++++++++++++++++++++++++++
plat/xen/drivers/net/netfront.h | 30 +++++++++
2 files changed, 138 insertions(+)
diff --git a/plat/xen/drivers/net/netfront.c
b/plat/xen/drivers/net/netfront.c
index ddbb4b70..3c936743 100644
--- a/plat/xen/drivers/net/netfront.c
+++ b/plat/xen/drivers/net/netfront.c
@@ -33,10 +33,12 @@
* THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
*/
+#include <string.h>
#include <uk/assert.h>
#include <uk/print.h>
#include <uk/alloc.h>
#include <uk/netdev_driver.h>
+#include <xen-x86/mm.h>
#include <xenbus/xenbus.h>
#include "netfront.h"
#include "netfront_xb.h"
@@ -49,6 +51,83 @@
static struct uk_alloc *drv_allocator;
+
+static void add_id_to_freelist(uint16_t id, uint16_t *freelist)
+{
+ freelist[id + 1] = freelist[0];
+ freelist[0] = id;
+}
+
+static uint16_t get_id_from_freelist(uint16_t *freelist)
+{
+ uint16_t id;
+
+ id = freelist[0];
+ freelist[0] = freelist[id + 1];
+ return id;
+}
+
+static struct uk_netdev_tx_queue *netfront_txq_setup(struct uk_netdev
*n,
+ uint16_t queue_id,
+ uint16_t nb_desc __unused,
+ struct uk_netdev_txqueue_conf *conf __unused)
+{
+ int rc;
+ struct netfront_dev *nfdev;
+ struct uk_netdev_tx_queue *txq;
+ netif_tx_sring_t *sring;
+
+ UK_ASSERT(n != NULL);
+
+ nfdev = to_netfront_dev(n);
+ if (queue_id >= nfdev->max_queue_pairs) {
+ uk_pr_err("Invalid queue identifier: %"__PRIu16"\n", queue_id);
+ return ERR2PTR(-EINVAL);
+ }
+
+ txq = &nfdev->txqs[queue_id];
+ UK_ASSERT(!txq->initialized);
+ txq->netfront_dev = nfdev;
+ txq->lqueue_id = queue_id;
+
+ /* Setup shared ring */
+ sring = uk_palloc(drv_allocator, 1);
Shouldn't the allocator from the txqueue_conf be used here while
setting up the ring
I see, I wasn't aware of that.
+ if (!sring)
+ return ERR2PTR(-ENOMEM);
+ memset(sring, 0, PAGE_SIZE);
+ SHARED_RING_INIT(sring);
+ FRONT_RING_INIT(&txq->ring, sring, PAGE_SIZE);
+ txq->ring_size = NET_TX_RING_SIZE;
+ txq->ring_ref = gnttab_grant_access(nfdev->xendev->otherend_id,
+ virt_to_mfn(sring), 0);
+ UK_ASSERT(txq->ring_ref != GRANT_INVALID_REF);
+
+ /* Setup event channel */
+ rc = evtchn_alloc_unbound(nfdev->xendev->otherend_id,
+ NULL, NULL,
+ &txq->evtchn);
+ if (rc) {
+ uk_pr_err("Error creating event channel: %d\n", rc);
+ gnttab_end_access(txq->ring_ref);
+ uk_pfree(drv_allocator, sring, 1);
+ return ERR2PTR(-rc);
+ }
+ /* Events are always disabled for tx queue */
+ mask_evtchn(txq->evtchn);
+
+ /* Initialize list of request ids */
+ uk_semaphore_init(&txq->sem, NET_TX_RING_SIZE);
+ for (uint16_t i = 0; i < NET_TX_RING_SIZE; i++) {
+ add_id_to_freelist(i, txq->freelist);
+ txq->gref[i] = GRANT_INVALID_REF;
+ }
+
+ txq->initialized = true;
+ nfdev->txqs_num++;
+
+ return txq;
+}
+
static int netfront_rxtx_alloc(struct netfront_dev *nfdev,
const struct uk_netdev_conf *conf)
{
@@ -90,6 +169,33 @@ err_free_txrx:
return rc;
}
+static int netfront_txq_info_get(struct uk_netdev *n,
+ uint16_t queue_id,
+ struct uk_netdev_queue_info *qinfo)
+{
+ struct netfront_dev *nfdev;
+ struct uk_netdev_tx_queue *txq;
+ int rc = 0;
+
+ UK_ASSERT(n != NULL);
+ UK_ASSERT(qinfo != NULL);
+
+ nfdev = to_netfront_dev(n);
+ if (unlikely(queue_id >= nfdev->txqs_num)) {
txqs_num is updated only after the queue is configured. The
txq_info_get should be called before the txq_configure. This
would provide the user of netdev the information needed to
configure the txq.
Yeah, nfdev->max_queue_pairs should make more sense.
+ uk_pr_err("Invalid queue_id %"__PRIu16"\n", queue_id);
+ rc = -EINVAL;
+ goto exit;
+ }
+ txq = &nfdev->txqs[queue_id];
+ qinfo->nb_min = txq->ring_size;
+ qinfo->nb_max = txq->ring_size;
+ qinfo->nb_align = PAGE_SIZE;
+ qinfo->nb_is_power_of_two = 1;
I noticed that there is a mis-understanding with nb_align and
nb_is_power_of_two. These parameters are related to the "number of
descriptors". They are describing restrictiond that apply for sizing a
queue. uknetdev does currently not have an align parameter for packet
buffers which we need for netfront, in fact.
Similar to ukblkdev, I added one called `ioalign` to netdev with the
patch series 1560:
https://patchwork.unikraft.org/project/unikraft/list/?series=1560
Something like this we need for properly support netfront. Sharan is
doing the review of 1560 currently.
Thanks,
Simon
+
+exit:
+ return rc;
+}
+
static int netfront_configure(struct uk_netdev *n,
const struct uk_netdev_conf *conf)
{
@@ -179,6 +285,8 @@ static unsigned int netfront_promisc_get(struct
uk_netdev *n)
static const struct uk_netdev_ops netfront_ops = {
.configure = netfront_configure,
+ .txq_configure = netfront_txq_setup,
+ .txq_info_get = netfront_txq_info_get,
.info_get = netfront_info_get,
.einfo_get = netfront_einfo_get,
.hwaddr_get = netfront_mac_get,
diff --git a/plat/xen/drivers/net/netfront.h
b/plat/xen/drivers/net/netfront.h
index a811b092..d3f603b3 100644
--- a/plat/xen/drivers/net/netfront.h
+++ b/plat/xen/drivers/net/netfront.h
@@ -37,11 +37,40 @@
#define __NETFRONT_H__
#include <uk/netdev.h>
+#include <uk/semaphore.h>
+#include <xen/io/netif.h>
+#include <common/gnttab.h>
+#include <common/events.h>
+
+
+#define NET_TX_RING_SIZE __CONST_RING_SIZE(netif_tx, PAGE_SIZE)
/**
* internal structure to represent the transmit queue.
*/
struct uk_netdev_tx_queue {
+ /* The netfront device */
+ struct netfront_dev *netfront_dev;
+ /* The libuknet queue identifier */
+ uint16_t lqueue_id;
+ /* True if initialized */
+ bool initialized;
+
+ /* Shared ring size */
+ uint16_t ring_size;
+ /* Shared ring */
+ netif_tx_front_ring_t ring;
+ /* Shared ring grant ref */
+ grant_ref_t ring_ref;
+ /* Queue event channel */
+ evtchn_port_t evtchn;
+
+ /* Free list protecting semaphore */
+ struct uk_semaphore sem;
+ /* Free list of transmitting request IDs */
+ uint16_t freelist[NET_TX_RING_SIZE + 1];
+ /* Grants for transmit buffers */
+ grant_ref_t gref[NET_TX_RING_SIZE];
};
/**
@@ -63,6 +92,7 @@ struct netfront_dev {
struct uk_netdev netdev;
/* List of the Rx/Tx queues */
+ uint16_t txqs_num;
struct uk_netdev_tx_queue *txqs;
struct uk_netdev_rx_queue *rxqs;
/* Maximum number of queue pairs */
|
