Re: [UNIKRAFT PATCH v4 05/12] plat/xen/drivers/net: Create netfront queues

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Hi Sharan,

Please see inline.

On 10/22/20 2:13 PM, Sharan Santhanam wrote:
> Hello Costin,
> 
> I have a minor comments inline but the rest of it fine.
> 
> Reviewed-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> 
> 
> Thanks & Regards
> 
> Sharan
> 
> On 8/13/20 10:53 AM, Costin Lupu wrote:
>> We continue with the device configuration by retrieving the Xenstore
>> information regarding the number of queues and their associated event
>> channels.
>> Netfront devices operate pairs of Rx/Tx queues and for notifications
>> we can
>> either use a single event channel per pair or split event channels.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> Signed-off-by: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
>> ---
>>   plat/xen/drivers/net/netfront.c    | 81 ++++++++++++++++++++++++++++++
>>   plat/xen/drivers/net/netfront.h    | 20 ++++++++
>>   plat/xen/drivers/net/netfront_xs.c | 18 ++++++-
>>   3 files changed, 117 insertions(+), 2 deletions(-)
>>
>> diff --git a/plat/xen/drivers/net/netfront.c
>> b/plat/xen/drivers/net/netfront.c
>> index 539e1cbc..ddbb4b70 100644
>> --- a/plat/xen/drivers/net/netfront.c
>> +++ b/plat/xen/drivers/net/netfront.c
>> @@ -49,6 +49,84 @@
>>     static struct uk_alloc *drv_allocator;
>>   +static int netfront_rxtx_alloc(struct netfront_dev *nfdev,
>> +        const struct uk_netdev_conf *conf)
>> +{
>> +    int rc = 0;
>> +
>> +    if (conf->nb_tx_queues != conf->nb_rx_queues) {
>> +        uk_pr_err("Different number of queues not supported\n");
>> +        rc = -ENOTSUP;
>> +        goto err_free_txrx;
>> +    }
>> +
>> +    nfdev->max_queue_pairs =
>> +        MIN(nfdev->max_queue_pairs, conf->nb_tx_queues);
> 
> Should we change the max_queue_pair to the user input or retain the max
> value provided by the system and use separate variable in the structure
> to store this?
> 

The max value provided by the system has no usage anymore after the user
configures it. The new value has to be published by the frontend in
order to be used by the system (i.e. netback driver) as well. However,
the current version doesn't do that, so I'll have to add the changes in
'09/12 plat/xen/drivers/net: Start netfront device' patch.

>> +
>> +    nfdev->txqs = uk_calloc(drv_allocator,
>> +        nfdev->max_queue_pairs, sizeof(*nfdev->txqs));
>> +    if (unlikely(!nfdev->txqs)) {
>> +        uk_pr_err("Failed to allocate memory for tx queues\n");
>> +        rc = -ENOMEM;
>> +        goto err_free_txrx;
>> +    }
>> +
>> +    nfdev->rxqs = uk_calloc(drv_allocator,
>> +        nfdev->max_queue_pairs, sizeof(*nfdev->rxqs));
>> +    if (unlikely(!nfdev->rxqs)) {
>> +        uk_pr_err("Failed to allocate memory for rx queues\n");
>> +        rc = -ENOMEM;
>> +        goto err_free_txrx;
>> +    }
>> +
>> +    return rc;
>> +
>> +err_free_txrx:
>> +    if (!nfdev->rxqs)
>> +        uk_free(drv_allocator, nfdev->rxqs);
>> +    if (!nfdev->txqs)
>> +        uk_free(drv_allocator, nfdev->txqs);
>> +
>> +    return rc;
>> +}
>> +
>> +static int netfront_configure(struct uk_netdev *n,
>> +        const struct uk_netdev_conf *conf)
>> +{
>> +    int rc;
>> +    struct netfront_dev *nfdev;
>> +
>> +    UK_ASSERT(n != NULL);
>> +    UK_ASSERT(conf != NULL);
>> +
>> +    nfdev = to_netfront_dev(n);
>> +
>> +    rc = netfront_rxtx_alloc(nfdev, conf);
>> +    if (rc != 0) {
>> +        uk_pr_err("Failed to allocate rx and tx rings %d\n", rc);
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    return rc;
>> +}
>> +
>> +static void netfront_info_get(struct uk_netdev *n,
>> +        struct uk_netdev_info *dev_info)
>> +{
>> +    struct netfront_dev *nfdev;
>> +
>> +    UK_ASSERT(n != NULL);
>> +    UK_ASSERT(dev_info != NULL);
>> +
>> +    nfdev = to_netfront_dev(n);
>> +    dev_info->max_rx_queues = nfdev->max_queue_pairs;
>> +    dev_info->max_tx_queues = nfdev->max_queue_pairs;
>> +    dev_info->max_mtu = nfdev->mtu;
>> +    dev_info->nb_encap_tx = 0;
>> +    dev_info->nb_encap_rx = 0;
>> +}
>> +
>>   static const void *netfront_einfo_get(struct uk_netdev *n,
>>           enum uk_netdev_einfo_type einfo_type)
>>   {
>> @@ -100,6 +178,8 @@ static unsigned int netfront_promisc_get(struct
>> uk_netdev *n)
>>   }
>>     static const struct uk_netdev_ops netfront_ops = {
>> +    .configure = netfront_configure,
>> +    .info_get = netfront_info_get,
>>       .einfo_get = netfront_einfo_get,
>>       .hwaddr_get = netfront_mac_get,
>>       .mtu_get = netfront_mtu_get,
>> @@ -121,6 +201,7 @@ static int netfront_add_dev(struct xenbus_device
>> *xendev)
>>         nfdev->xendev = xendev;
>>       nfdev->mtu = ETH_PKT_PAYLOAD_LEN;
>> +    nfdev->max_queue_pairs = 1;
>>         /* Xenbus initialization */
>>       rc = netfront_xb_init(nfdev, drv_allocator);
>> diff --git a/plat/xen/drivers/net/netfront.h
>> b/plat/xen/drivers/net/netfront.h
>> index 0cc8230b..a811b092 100644
>> --- a/plat/xen/drivers/net/netfront.h
>> +++ b/plat/xen/drivers/net/netfront.h
>> @@ -38,6 +38,18 @@
>>     #include <uk/netdev.h>
>>   +/**
>> + * internal structure to represent the transmit queue.
>> + */
>> +struct uk_netdev_tx_queue {
>> +};
>> +
>> +/**
>> + * internal structure to represent the receive queue.
>> + */
>> +struct uk_netdev_rx_queue {
>> +};
>> +
>>   struct xs_econf {
>>       char *ipv4addr;
>>       char *ipv4mask;
>> @@ -50,6 +62,14 @@ struct netfront_dev {
>>       /* Network device */
>>       struct uk_netdev netdev;
>>   +    /* List of the Rx/Tx queues */
>> +    struct uk_netdev_tx_queue *txqs;
>> +    struct uk_netdev_rx_queue *rxqs;
>> +    /* Maximum number of queue pairs */
>> +    uint16_t  max_queue_pairs;
>> +    /* True if using split event channels */
>> +    bool split_evtchn;
>> +
>>       /* Configuration parameters */
>>       struct xs_econf econf;
>>   diff --git a/plat/xen/drivers/net/netfront_xs.c
>> b/plat/xen/drivers/net/netfront_xs.c
>> index 48f01b34..1bde44e0 100644
>> --- a/plat/xen/drivers/net/netfront_xs.c
>> +++ b/plat/xen/drivers/net/netfront_xs.c
>> @@ -122,7 +122,7 @@ out_err:
>>   int netfront_xb_init(struct netfront_dev *nfdev, struct uk_alloc *a)
>>   {
>>       struct xenbus_device *xendev;
>> -    char *mac_str, *p, *ip_str;
>> +    char *mac_str, *p, *ip_str, *int_str;
>>       int rc;
>>         UK_ASSERT(nfdev != NULL);
>> @@ -173,7 +173,21 @@ int netfront_xb_init(struct netfront_dev *nfdev,
>> struct uk_alloc *a)
>>           goto no_conf;
>>       free(ip_str);
>>   -    /* TODO spit event channels */
>> +    /* maximum queues number */
>> +    int_str = xs_read(XBT_NIL, xendev->otherend,
>> +        "multi-queue-max-queues");
>> +    if (!PTRISERR(int_str)) {
>> +        nfdev->max_queue_pairs = (uint16_t) strtoul(int_str, NULL, 10);
>> +        free(int_str);
>> +    }
>> +
>> +    /* spit event channels */
>> +    int_str = xs_read(XBT_NIL, xendev->otherend,
>> +        "feature-split-event-channels");
>> +    if (!PTRISERR(int_str)) {
>> +        nfdev->split_evtchn = (bool) strtoul(int_str, NULL, 10);
>> +        free(int_str);
>> +    }
>>         /* TODO netmap */
>>   
>