Re: [dpdk-dev] [PATCH v2 1/6] net/af_xdp: introduce AF XDP PMD driver

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

Thanks for your comments.

On 03/20, David Marchand wrote:
>On Tue, Mar 19, 2019 at 8:17 AM Xiaolong Ye <xiaolong.ye@intel.com> wrote:
>
>> diff --git a/doc/guides/nics/features/af_xdp.ini
>> b/doc/guides/nics/features/af_xdp.ini
>> new file mode 100644
>> index 000000000..7b8fcce00
>> --- /dev/null
>> +++ b/doc/guides/nics/features/af_xdp.ini
>> @@ -0,0 +1,11 @@
>> +;
>> +; Supported features of the 'af_xdp' network poll mode driver.
>> +;
>> +; Refer to default.ini for the full list of available PMD features.
>> +;
>> +[Features]
>> +Link status          = Y
>> +MTU update           = Y
>> +Promiscuous mode     = Y
>> +Stats per queue      = Y
>> +x86-64               = Y
>>
>
>Is there really a limitation on x86?

I think no, just don't have a chance to try it on a x86-32 machine.

>
>
>diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> index 502869a87..5d401b8c5 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -9,6 +9,7 @@ ifeq ($(CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD),d)
>>  endif
>>
>>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_AF_PACKET) += af_packet
>> +DIRS-$(CONFIG_RTE_LIBRTE_PMD_AF_XDP) += af_xdp
>>  DIRS-$(CONFIG_RTE_LIBRTE_ARK_PMD) += ark
>>  DIRS-$(CONFIG_RTE_LIBRTE_ATLANTIC_PMD) += atlantic
>>  DIRS-$(CONFIG_RTE_LIBRTE_AVP_PMD) += avp
>> diff --git a/drivers/net/af_xdp/Makefile b/drivers/net/af_xdp/Makefile
>> new file mode 100644
>> index 000000000..6cf0ed7db
>> --- /dev/null
>> +++ b/drivers/net/af_xdp/Makefile
>> @@ -0,0 +1,33 @@
>> +# SPDX-License-Identifier: BSD-3-Clause
>> +# Copyright(c) 2018 Intel Corporation
>>
>
>2018? 2019?

Will correct it to 2019.

>
>+
>> +include $(RTE_SDK)/mk/rte.vars.mk
>> +
>> +#
>> +# library name
>> +#
>> +LIB = librte_pmd_af_xdp.a
>> +
>> +EXPORT_MAP := rte_pmd_af_xdp_version.map
>> +
>> +LIBABIVER := 1
>> +
>> +CFLAGS += -O3
>> +
>> +# require kernel version >= v5.1-rc1
>> +LINUX_VERSION := $(shell uname -r)
>> +CFLAGS += -I/lib/modules/$(LINUX_VERSION)/build/tools/include
>> +CFLAGS += -I/lib/modules/$(LINUX_VERSION)/build/tools/lib/bpf
>>
>
>We can reuse RTE_KERNELDIR here (even if the docs state that this was to
>build kmods so far).

Will do.

>
>
>> 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..96dedc0c4
>> --- /dev/null
>> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>>
>
>[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_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];
>>
>
>You forgot to remove stats->q_errors[i];
>
>+               stats->oerrors += internals->tx_queues[i].err_pkts;

My bad, will remove it in next version.


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