Re: [UNIKRAFT PATCH v4 10/12] plat/xen/drivers/net: Add transmit operation

[Costin Lupu <costin.lupu@xxxxxxxxx>]

On 8/20/20 6:49 PM, Simon Kuenzer wrote:
> On 13.08.20 10:53, Costin Lupu wrote:
>> Whenever a packet is transmitted, the request describing it is written
>> in the
>> shared ring. The packet itself is written in a page referenced by the
>> request
>> and shared, as well, with the backend. At the end of each transmit
>> operation, a
>> cleanup operation is performed free'ing the resources allocated for the
>> previously transmitted packets.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> Signed-off-by: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
>> ---
>>   plat/xen/drivers/net/netfront.c | 102 ++++++++++++++++++++++++++++++++
>>   1 file changed, 102 insertions(+)
>>
>> diff --git a/plat/xen/drivers/net/netfront.c
>> b/plat/xen/drivers/net/netfront.c
>> index 858d124a..f2b81329 100644
>> --- a/plat/xen/drivers/net/netfront.c
>> +++ b/plat/xen/drivers/net/netfront.c
>> @@ -39,6 +39,7 @@
>>   #include <uk/alloc.h>
>>   #include <uk/netdev_driver.h>
>>   #include <xen-x86/mm.h>
>> +#include <xen-x86/irq.h>
>>   #include <xenbus/xenbus.h>
>>   #include "netfront.h"
>>   #include "netfront_xb.h"
>> @@ -78,6 +79,106 @@ static uint16_t get_id_from_freelist(uint16_t
>> *freelist)
>>       return id;
>>   }
>>   +static int network_tx_buf_gc(struct uk_netdev_tx_queue *txq)
>> +{
>> +    RING_IDX prod, cons;
>> +    netif_tx_response_t *tx_rsp;
>> +    uint16_t id;
>> +    bool more_to_do;
>> +    int count = 0;
>> +
>> +    do {
>> +        prod = txq->ring.sring->rsp_prod;
>> +        rmb(); /* Ensure we see responses up to 'rp'. */
>> +
>> +        for (cons = txq->ring.rsp_cons; cons != prod; cons++) {
>> +            tx_rsp = RING_GET_RESPONSE(&txq->ring, cons);
>> +
>> +            if (tx_rsp->status == NETIF_RSP_NULL)
>> +                continue;
>> +
>> +            if (tx_rsp->status == NETIF_RSP_ERROR)
>> +                uk_pr_err("packet error\n");
> 
> "netdev%u: Transmission error on txq %u\n" ?
> 

Ack.

>> +
>> +            id  = tx_rsp->id;
>> +            UK_ASSERT(id < NET_TX_RING_SIZE);
>> +
>> +            gnttab_end_access(txq->gref[id]);
>> +            txq->gref[id] = GRANT_INVALID_REF;
>> +
>> +            add_id_to_freelist(id, txq->freelist);
>> +            uk_semaphore_up(&txq->sem);
>> +
>> +            count++;
>> +        }
>> +
>> +        txq->ring.rsp_cons = prod;
>> +
>> +        RING_FINAL_CHECK_FOR_RESPONSES(&txq->ring, more_to_do);
>> +    } while (more_to_do);
>> +
>> +    return count;
>> +}
>> +
>> +static int netfront_xmit(struct uk_netdev *n,
>> +        struct uk_netdev_tx_queue *txq,
>> +        struct uk_netbuf *pkt)
>> +{
>> +    struct netfront_dev *nfdev;
>> +    unsigned long flags;
>> +    uint16_t id;
>> +    RING_IDX req_prod;
>> +    netif_tx_request_t *tx_req;
>> +    int notify;
>> +    int status = 0, count;
>> +
>> +    UK_ASSERT(n != NULL);
>> +    UK_ASSERT(txq != NULL);
>> +    UK_ASSERT(pkt != NULL);
>> +    UK_ASSERT(pkt->len < PAGE_SIZE);
>> +
>> +    nfdev = to_netfront_dev(n);
>> +
>> +    /* get request id */
>> +    uk_semaphore_down(&txq->sem);
>> +    local_irq_save(flags);
>> +    id = get_id_from_freelist(txq->freelist);
> 
> What happens when we are out of space and the ring is full?
> The xmit function should be non-blocking so that we can use it in
> polling mode. We should return in such a case that the transmission
> queue is currently full...
> 

Ack.

>> +    local_irq_restore(flags);
>> +
>> +    /* get request */
>> +    req_prod = txq->ring.req_prod_pvt;
>> +    tx_req = RING_GET_REQUEST(&txq->ring, req_prod); > +
>> +    /* setup grant for buffer data */
>> +    txq->gref[id] = tx_req->gref =
>> +        gnttab_grant_access(nfdev->xendev->otherend_id,
>> +            virt_to_mfn(pkt->data), 1);
>> +    UK_ASSERT(tx_req->gref != GRANT_INVALID_REF);
>> +
>> +    tx_req->offset = 0;
>> +    tx_req->size = (uint16_t) pkt->len;
>> +    tx_req->flags = 0;
>> +    tx_req->id = id;
>> +
>> +    txq->ring.req_prod_pvt = req_prod + 1;
>> +    wmb(); /* Ensure backend sees requests */
>> +
>> +    RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&txq->ring, notify);
>> +    if (notify)
>> +        notify_remote_via_evtchn(txq->evtchn); > +
>> +    status |= UK_NETDEV_STATUS_SUCCESS;
>> +
>> +    /* some cleanup */
>> +    local_irq_save(flags);
>> +    count = network_tx_buf_gc(txq);
> 
> The clean-up should happen before enqueuing the given packet to the
> transmit queue, it is the first thing of xmit(). Since we anyway need to
> do it and we do not have an advantage by delaying this operation. 

Of course there is an advantage. You push the packets in order for them
to be processed ASAP by the backend and deal with cleaning up
afterwards. It's a kind of parallelization. Otherwise you would delay
the transmissions.

> We can also deal much better with full rings. Otherwise we
> unnecessarily drop a packets for transmission although there would
> have been space when clean-up finished. After calling cleaning you
> can check if there is space on the ring and return if we run out of
> it (use an `unlikely` to the condition so that we speed up the
> successful sending case).
> 

The only scenario when this would solve anything would be when you have
the ring full and just before you decide to send a new packet the
backend starts processing your previous transmitted packets, leaving you
at least one free slot. There are 2 unlikely events here: first, the
ring is full, and second, the backend starts to process again when the
ring gets full (for the latter event, it's unlikely that the backend
will wake up given that it couldn't process 256 transmitted packets,
where 256 is the current number of slots). And for this very unlikely
context you would introduce a delay for all the transmissions if we
followed your suggestion.

Anyway, both Linux and FreeBSD do the cleanup after pushing the
requests, so I don't think we have much more to say about this.


Btw, I'm waiting for reviews on the other patches as well before I send
the v5.


>> +    local_irq_restore(flags);
>> +
>> +    status |= likely(count > 0) ? UK_NETDEV_STATUS_MORE : 0x0;
>> +
>> +    return status;
>> +}
>> +
>>   static int netfront_rxq_enqueue(struct uk_netdev_rx_queue *rxq,
>>           struct uk_netbuf *netbuf)
>>   {
>> @@ -598,6 +699,7 @@ static int netfront_add_dev(struct xenbus_device
>> *xendev)
>>       }
>>         /* register netdev */
>> +    nfdev->netdev.tx_one = netfront_xmit;
>>       nfdev->netdev.ops = &netfront_ops;
>>       rc = uk_netdev_drv_register(&nfdev->netdev, drv_allocator,
>> DRIVER_NAME);
>>       if (rc < 0) {
>>
>