Re: [UNIKRAFT PATCH v4 06/12] plat/xen/drivers/net: Configure netfront tx queue

[Costin Lupu <costin.lupu@xxxxxxxxx>]

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;
>> +
>> +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 */
>