Re: [PATCH v1 1/6] net/af_xdp: introduce AF_XDP PMD driver

[Ye Xiaolong <xiaolong.ye@intel.com>]

Hi, David

Thanks for your review.

On 03/05, David Marchand wrote:
>On Fri, Mar 1, 2019 at 9:13 AM Xiaolong Ye <xiaolong.ye@intel.com> wrote:
>
>> diff --git a/doc/guides/rel_notes/release_18_11.rst
>> b/doc/guides/rel_notes/release_18_11.rst
>> index 65bab557d..e0918441a 100644
>> --- a/doc/guides/rel_notes/release_18_11.rst
>> +++ b/doc/guides/rel_notes/release_18_11.rst
>> @@ -229,6 +229,13 @@ New Features
>>    The AESNI MB PMD has been updated with additional support for the
>> AES-GCM
>>    algorithm.
>>
>> +* **Added the AF_XDP PMD.**
>> +
>> +  Added a Linux-specific PMD driver for AF_XDP, it can create the AF_XDP
>> socket
>> +  and bind it to a specific netdev queue, it allows a DPDK application to
>> send
>> +  and receive raw packets through the socket which would bypass the kernel
>> +  network stack to achieve high performance packet processing.
>> +
>>
>
>Should be in 19.05 release notes.

Ooops, my bad, will swith to 19.05 in next version.

>
>
>diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> new file mode 100644
>> index 000000000..6de769650
>> --- /dev/null
>> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>>
>> [snip]
>
>+struct pkt_rx_queue {
>> +       struct xsk_ring_cons rx;
>> +       struct xsk_umem_info *umem;
>> +       struct xsk_socket *xsk;
>> +       struct rte_mempool *mb_pool;
>> +
>> +       unsigned long rx_pkts;
>> +       unsigned long rx_bytes;
>> +       unsigned long rx_dropped;
>>
>
>ethdev stats are uint64_t, why declare those internal stats as unsigned
>long ?

You are right, should use uint64_t instead.

>
>+
>> +       struct pkt_tx_queue *pair;
>> +       uint16_t queue_idx;
>> +};
>> +
>> +struct pkt_tx_queue {
>> +       struct xsk_ring_prod tx;
>> +
>> +       unsigned long tx_pkts;
>> +       unsigned long err_pkts;
>> +       unsigned long tx_bytes;
>>
>
>Idem.

Will change to uint64_t.

>
>+
>> +       struct pkt_rx_queue *pair;
>> +       uint16_t queue_idx;
>> +};
>>
>
>[snip]
>
>+static int
>> +eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>> +{
>> +       struct pmd_internals *internals = dev->data->dev_private;
>> +       struct xdp_statistics xdp_stats;
>> +       struct pkt_rx_queue *rxq;
>> +       socklen_t optlen;
>> +       int i;
>> +
>> +       optlen = sizeof(struct xdp_statistics);
>> +       for (i = 0; i < dev->data->nb_rx_queues; i++) {
>> +               rxq = &internals->rx_queues[i];
>> +               stats->q_ipackets[i] = internals->rx_queues[i].rx_pkts;
>> +               stats->q_ibytes[i] = internals->rx_queues[i].rx_bytes;
>> +
>> +               stats->q_opackets[i] = internals->tx_queues[i].tx_pkts;
>> +               stats->q_errors[i] = internals->tx_queues[i].err_pkts;
>>
>
>q_errors[] are for reception errors, see the patchset I sent:
>http://mails.dpdk.org/archives/dev/2019-March/125703.html
>
>If you want per queue stats, use xstats.
>You can still account those errors in the global stats->oerrors below.
>
>+               stats->q_obytes[i] = internals->tx_queues[i].tx_bytes;
>> +
>> +               stats->ipackets += stats->q_ipackets[i];
>> +               stats->ibytes += stats->q_ibytes[i];
>> +               stats->imissed += internals->rx_queues[i].rx_dropped;
>> +               getsockopt(xsk_socket__fd(rxq->xsk), SOL_XDP,
>> XDP_STATISTICS,
>> +                               &xdp_stats, &optlen);
>> +               stats->imissed += xdp_stats.rx_dropped;
>> +
>> +               stats->opackets += stats->q_opackets[i];
>> +               stats->oerrors += stats->q_errors[i];
>>
>
>-               stats->oerrors += stats->q_errors[i];
>+               stats->oerrors += internals->tx_queues[i].err_pkts;

will correct according to your patch.

Thanks,
Xiaolong

>
>>
>> +               stats->obytes += stats->q_obytes[i];
>> +       }
>> +
>> +       return 0;
>> +}
>>
>>
>
>-- 
>David Marchand