* Re: [PATCH 26/28] net/virtio: use eal I/O device memory read/write API
From: Yuanhan Liu @ 2016-12-14 2:46 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, konstantin.ananyev, thomas.monjalon, bruce.richardson,
jianbo.liu, viktorin, Santosh Shukla, Huawei Xie
In-Reply-To: <1481680558-4003-27-git-send-email-jerin.jacob@caviumnetworks.com>
On Wed, Dec 14, 2016 at 07:25:56AM +0530, Jerin Jacob wrote:
> From: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>
> Replace the raw I/O device memory read/write access with eal
> abstraction for I/O device memory read/write access to fix
> portability issues across different architectures.
I think these APIs are good:
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: Huawei Xie <huawei.xie@intel.com>
> CC: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Not a big deal, but I think we normally put the 'Cc' above the SoB.
--yliu
^ permalink raw reply
* Re: [PATCH 00/28] introduce I/O device memory read/write operations
From: Yuanhan Liu @ 2016-12-14 2:53 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, konstantin.ananyev, thomas.monjalon, bruce.richardson,
jianbo.liu, viktorin
In-Reply-To: <1481680558-4003-1-git-send-email-jerin.jacob@caviumnetworks.com>
On Wed, Dec 14, 2016 at 07:25:30AM +0530, Jerin Jacob wrote:
> patchset 14-28: Replace the raw readl/writel in the drivers with
> new rte_read[b/w/l/q], rte_write[b/w/l/q] eal abstraction
Instead of rte_read[b/w/l/q], there is another typical naming style:
rte_read[8/16/32/64]. Any preferences? If you ask me, I'd prefer the
later.
--yliu
^ permalink raw reply
* Re: [PATCH 27/28] net/vmxnet3: use eal I/O device memory read/write API
From: Yuanhan Liu @ 2016-12-14 2:55 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, konstantin.ananyev, thomas.monjalon, bruce.richardson,
jianbo.liu, viktorin, Santosh Shukla, Yong Wang
In-Reply-To: <1481680558-4003-28-git-send-email-jerin.jacob@caviumnetworks.com>
On Wed, Dec 14, 2016 at 07:25:57AM +0530, Jerin Jacob wrote:
> From: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>
> Replace the raw I/O device memory read/write access with eal
> abstraction for I/O device memory read/write access to fix
> portability issues across different architectures.
>
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: Yong Wang <yongwang@vmware.com>
> ---
> drivers/net/vmxnet3/vmxnet3_ethdev.h | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.h b/drivers/net/vmxnet3/vmxnet3_ethdev.h
> index 7d3b11e..5b6501b 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.h
> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.h
> @@ -34,6 +34,8 @@
> #ifndef _VMXNET3_ETHDEV_H_
> #define _VMXNET3_ETHDEV_H_
>
> +#include <rte_io.h>
> +
> #define VMXNET3_MAX_MAC_ADDRS 1
>
> /* UPT feature to negotiate */
> @@ -120,7 +122,11 @@ struct vmxnet3_hw {
>
> /* Config space read/writes */
>
> -#define VMXNET3_PCI_REG(reg) (*((volatile uint32_t *)(reg)))
> +#define VMXNET3_PCI_REG(reg) ({ \
> + uint32_t __val; \
> + __val = rte_readl(reg); \
> + __val; \
> +})
Why not simply using rte_readl directly?
#define VMXNET3_PCI_REG(reg) rte_readl(reg)
>
> static inline uint32_t
> vmxnet3_read_addr(volatile void *addr)
> @@ -128,9 +134,9 @@ vmxnet3_read_addr(volatile void *addr)
> return VMXNET3_PCI_REG(addr);
> }
>
> -#define VMXNET3_PCI_REG_WRITE(reg, value) do { \
> - VMXNET3_PCI_REG((reg)) = (value); \
> -} while(0)
> +#define VMXNET3_PCI_REG_WRITE(reg, value) ({ \
> + rte_writel(value, reg); \
> +})
I think this could be done in one line.
--yliu
^ permalink raw reply
* Re: [PATCH 26/28] net/virtio: use eal I/O device memory read/write API
From: Yuanhan Liu @ 2016-12-14 3:02 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, konstantin.ananyev, thomas.monjalon, bruce.richardson,
jianbo.liu, viktorin, Santosh Shukla, Huawei Xie
In-Reply-To: <1481680558-4003-27-git-send-email-jerin.jacob@caviumnetworks.com>
On Wed, Dec 14, 2016 at 07:25:56AM +0530, Jerin Jacob wrote:
> * Following macros are derived from linux/pci_regs.h, however,
> * we can't simply include that header here, as there is no such
> @@ -320,37 +322,37 @@ static const struct virtio_pci_ops legacy_ops = {
> static inline uint8_t
> io_read8(uint8_t *addr)
> {
> - return *(volatile uint8_t *)addr;
> + return rte_readb(addr);
> }
Oh, one more comments: why not replacing io_read8 with rte_readb(),
and do similar for others? Then we don't have to define those wrappers.
I think you can also do something similar for other patches?
--yliu
>
> static inline void
> io_write8(uint8_t val, uint8_t *addr)
> {
> - *(volatile uint8_t *)addr = val;
> + rte_writeb(val, addr);
> }
>
> static inline uint16_t
> io_read16(uint16_t *addr)
> {
> - return *(volatile uint16_t *)addr;
> + return rte_readw(addr);
> }
>
> static inline void
> io_write16(uint16_t val, uint16_t *addr)
> {
> - *(volatile uint16_t *)addr = val;
> + rte_writew(val, addr);
> }
>
> static inline uint32_t
> io_read32(uint32_t *addr)
> {
> - return *(volatile uint32_t *)addr;
> + return rte_readl(addr);
> }
>
> static inline void
> io_write32(uint32_t val, uint32_t *addr)
> {
> - *(volatile uint32_t *)addr = val;
> + rte_writel(val, addr);
> }
>
> static inline void
> --
> 2.5.5
^ permalink raw reply
* Re: [PATCH v4] vhost: allow for many vhost user ports
From: Yuanhan Liu @ 2016-12-14 3:25 UTC (permalink / raw)
To: Jan Wickbom; +Cc: dev, patrik.r.andersson
In-Reply-To: <1481635187-12624-1-git-send-email-jan.wickbom@ericsson.com>
On Tue, Dec 13, 2016 at 02:19:47PM +0100, Jan Wickbom wrote:
> +
> + poll(pfdset->rwfds, numfds, 1000 /* millisecs */);
> +
> + for (i = 0; i < numfds; ) {
> pthread_mutex_lock(&pfdset->fd_mutex);
> +
> pfdentry = &pfdset->fd[i];
> fd = pfdentry->fd;
> + pfd = &pfdset->rwfds[i];
> +
> + if (fd < 0) {
> + /* Removed during poll */
> + /* shrink first, last migth be deleted*/
> +
> + fdset_shrink(pfdset);
> + fdset_move_last(pfdset, i);
> + fdset_shrink(pfdset);
> +
> + pthread_mutex_unlock(&pfdset->fd_mutex);
This patch looks better, but as said, I will not do shrink in the
event processing loop: I would simply set a flag, something like
"need_shrink" here, and do the shrink outside this for loop.
> + if (remove1 || remove2) {
> + pthread_mutex_lock(&pfdset->fd_mutex);
> +
> + /* shrink first, last migth be deleted*/
> + fdset_shrink(pfdset);
> + fdset_move_last(pfdset, i);
> + fdset_shrink(pfdset);
> +
> + pthread_mutex_unlock(&pfdset->fd_mutex);
> +
> + continue;
Same here: just sets a flag.
> + }
> +
> + i++;
> }
> +
> + /* fdset_del do not shrink, pack eventual remainings of array */
> + pthread_mutex_lock(&pfdset->fd_mutex);
> +
> + fdset_shrink(pfdset);
> +
> + for ( ; i < pfdset->num; i++) {
> + pfdentry = &pfdset->fd[i];
> +
> + if (pfdentry->fd < 0) {
> + fdset_move_last(pfdset, i);
> + fdset_shrink(pfdset);
> + }
> + }
And yes, do the shrink here (when the shrink flag is set). But I would
simply call fdset_shrink() here (without the for loop), and let the
fdset_shrink() to handle the details: it could either be a swap with
last __valid__ entry, or simply a memmov.
That said, if you prefer to choose fdset_move_last(), fine, invoke it
inside fdset_shrink() then. Let fdset_shrink be able to remove an fd
in the middle.
> +
> + pthread_mutex_unlock(&pfdset->fd_mutex);
> }
> }
> diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
> index bd66ed1..03e7881 100644
> --- a/lib/librte_vhost/fd_man.h
> +++ b/lib/librte_vhost/fd_man.h
> @@ -35,6 +35,7 @@
> #define _FD_MAN_H_
> #include <stdint.h>
> #include <pthread.h>
> +#include <poll.h>
>
> #define MAX_FDS 1024
>
> @@ -49,9 +50,10 @@ struct fdentry {
> };
>
> struct fdset {
> + struct pollfd rwfds[MAX_FDS];
> struct fdentry fd[MAX_FDS];
> pthread_mutex_t fd_mutex;
> - int num; /* current fd number of this fdset */
> + int num; /* highest index occupied in fd array + 1 */
I don't see the comment change makes it more readable.
--yliu
^ permalink raw reply
* Re: [PATCH v2 12/12] drivers: update PMDs to use rte_driver probe and remove
From: Shreyansh Jain @ 2016-12-14 5:11 UTC (permalink / raw)
To: Andrew Rybchenko, dev
Cc: david.marchand, thomas.monjalon, ferruh.yigit, jianbo.liu
In-Reply-To: <44213d68-1e87-c464-549a-274e389b9c0f@solarflare.com>
On Tuesday 13 December 2016 07:22 PM, Andrew Rybchenko wrote:
> On 12/13/2016 04:37 PM, Shreyansh Jain wrote:
>> These callbacks now act as first layer of PCI interfaces from the Bus.
>> Bus probe would enter the PMDs through the rte_driver->probe/remove
>> callbacks, falling to rte_xxx_driver->probe/remove (Currently, all the
>> drivers are rte_pci_driver).
>
> I think similar changes in drivers/net/sfc/sfc_ethdev.c (already in
> dpdk-next-net) are required as well.
Thanks for highlighting.
IIRC, similar point was highlighted by Ferruh as well.
Indeed a related change is required for all PMDs (PCI, for now) - if
this change set (or a variation) is accepted.
So, just to clarify: This patch series is based on master (dpdk). How
should such changes (for drivers in dpdk-next-net) be highlighted? Or,
how do we track such merges?
One obvious thing I can see is that those PMDs, which are in
dpdk-next-net, are changed by their respective authors if this series
(or similar changes by other series) are absorbed in master and when
their series is merged upstream.
Any other expected/assumed way?
>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> ---
>> drivers/net/bnx2x/bnx2x_ethdev.c | 8 ++++++++
>> drivers/net/bnxt/bnxt_ethdev.c | 4 ++++
>> drivers/net/cxgbe/cxgbe_ethdev.c | 4 ++++
>> drivers/net/e1000/em_ethdev.c | 4 ++++
>> drivers/net/e1000/igb_ethdev.c | 8 ++++++++
>> drivers/net/ena/ena_ethdev.c | 4 ++++
>> drivers/net/enic/enic_ethdev.c | 4 ++++
>> drivers/net/fm10k/fm10k_ethdev.c | 4 ++++
>> drivers/net/i40e/i40e_ethdev.c | 4 ++++
>> drivers/net/i40e/i40e_ethdev_vf.c | 4 ++++
>> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++++++
>> drivers/net/mlx4/mlx4.c | 4 +++-
>> drivers/net/mlx5/mlx5.c | 1 +
>> drivers/net/nfp/nfp_net.c | 4 ++++
>> drivers/net/qede/qede_ethdev.c | 8 ++++++++
>> drivers/net/szedata2/rte_eth_szedata2.c | 4 ++++
>> drivers/net/thunderx/nicvf_ethdev.c | 4 ++++
>> drivers/net/virtio/virtio_ethdev.c | 2 ++
>> drivers/net/vmxnet3/vmxnet3_ethdev.c | 4 ++++
>> 19 files changed, 86 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bnx2x/bnx2x_ethdev.c
>> b/drivers/net/bnx2x/bnx2x_ethdev.c
>> index 0eae433..9f3b3f2 100644
>> --- a/drivers/net/bnx2x/bnx2x_ethdev.c
>> +++ b/drivers/net/bnx2x/bnx2x_ethdev.c
>> @@ -618,6 +618,10 @@ eth_bnx2xvf_dev_init(struct rte_eth_dev *eth_dev)
>> static struct eth_driver rte_bnx2x_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_bnx2x_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
>> .probe = rte_eth_dev_pci_probe,
>> @@ -632,6 +636,10 @@ static struct eth_driver rte_bnx2x_pmd = {
>> */
>> static struct eth_driver rte_bnx2xvf_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_bnx2xvf_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
>> .probe = rte_eth_dev_pci_probe,
>> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
>> b/drivers/net/bnxt/bnxt_ethdev.c
>> index 035fe07..c8671c8 100644
>> --- a/drivers/net/bnxt/bnxt_ethdev.c
>> +++ b/drivers/net/bnxt/bnxt_ethdev.c
>> @@ -1160,6 +1160,10 @@ bnxt_dev_uninit(struct rte_eth_dev *eth_dev) {
>> static struct eth_driver bnxt_rte_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = bnxt_pci_id_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING |
>> RTE_PCI_DRV_DETACHABLE | RTE_PCI_DRV_INTR_LSC,
>> diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c
>> b/drivers/net/cxgbe/cxgbe_ethdev.c
>> index b7f28eb..67714fa 100644
>> --- a/drivers/net/cxgbe/cxgbe_ethdev.c
>> +++ b/drivers/net/cxgbe/cxgbe_ethdev.c
>> @@ -1039,6 +1039,10 @@ static int eth_cxgbe_dev_init(struct
>> rte_eth_dev *eth_dev)
>> static struct eth_driver rte_cxgbe_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = cxgb4_pci_tbl,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
>> .probe = rte_eth_dev_pci_probe,
>> diff --git a/drivers/net/e1000/em_ethdev.c
>> b/drivers/net/e1000/em_ethdev.c
>> index aee3d34..7be5da3 100644
>> --- a/drivers/net/e1000/em_ethdev.c
>> +++ b/drivers/net/e1000/em_ethdev.c
>> @@ -391,6 +391,10 @@ eth_em_dev_uninit(struct rte_eth_dev *eth_dev)
>> static struct eth_driver rte_em_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_em_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
>> RTE_PCI_DRV_DETACHABLE,
>> diff --git a/drivers/net/e1000/igb_ethdev.c
>> b/drivers/net/e1000/igb_ethdev.c
>> index 2fddf0c..70dd24c 100644
>> --- a/drivers/net/e1000/igb_ethdev.c
>> +++ b/drivers/net/e1000/igb_ethdev.c
>> @@ -1078,6 +1078,10 @@ eth_igbvf_dev_uninit(struct rte_eth_dev *eth_dev)
>> static struct eth_driver rte_igb_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_igb_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
>> RTE_PCI_DRV_DETACHABLE,
>> @@ -1094,6 +1098,10 @@ static struct eth_driver rte_igb_pmd = {
>> */
>> static struct eth_driver rte_igbvf_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_igbvf_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
>> .probe = rte_eth_dev_pci_probe,
>> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
>> index ab9a178..54fc8de 100644
>> --- a/drivers/net/ena/ena_ethdev.c
>> +++ b/drivers/net/ena/ena_ethdev.c
>> @@ -1705,6 +1705,10 @@ static uint16_t eth_ena_xmit_pkts(void
>> *tx_queue, struct rte_mbuf **tx_pkts,
>> static struct eth_driver rte_ena_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_ena_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING,
>> .probe = rte_eth_dev_pci_probe,
>> diff --git a/drivers/net/enic/enic_ethdev.c
>> b/drivers/net/enic/enic_ethdev.c
>> index 2b154ec..c2783db 100644
>> --- a/drivers/net/enic/enic_ethdev.c
>> +++ b/drivers/net/enic/enic_ethdev.c
>> @@ -634,6 +634,10 @@ static int eth_enicpmd_dev_init(struct
>> rte_eth_dev *eth_dev)
>> static struct eth_driver rte_enic_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_enic_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
>> .probe = rte_eth_dev_pci_probe,
>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
>> b/drivers/net/fm10k/fm10k_ethdev.c
>> index 923690c..d1a2efa 100644
>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>> @@ -3061,6 +3061,10 @@ static const struct rte_pci_id
>> pci_id_fm10k_map[] = {
>> static struct eth_driver rte_pmd_fm10k = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_fm10k_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
>> RTE_PCI_DRV_DETACHABLE,
>> diff --git a/drivers/net/i40e/i40e_ethdev.c
>> b/drivers/net/i40e/i40e_ethdev.c
>> index 67778ba..9c5d50f 100644
>> --- a/drivers/net/i40e/i40e_ethdev.c
>> +++ b/drivers/net/i40e/i40e_ethdev.c
>> @@ -670,6 +670,10 @@ static const struct rte_i40e_xstats_name_off
>> rte_i40e_txq_prio_strings[] = {
>> static struct eth_driver rte_i40e_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_i40e_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
>> RTE_PCI_DRV_DETACHABLE,
>> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
>> b/drivers/net/i40e/i40e_ethdev_vf.c
>> index aa306d6..10bf6ab 100644
>> --- a/drivers/net/i40e/i40e_ethdev_vf.c
>> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
>> @@ -1527,6 +1527,10 @@ i40evf_dev_uninit(struct rte_eth_dev *eth_dev)
>> */
>> static struct eth_driver rte_i40evf_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_i40evf_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
>> .probe = rte_eth_dev_pci_probe,
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index edc9b22..80ee232 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -1564,6 +1564,10 @@ eth_ixgbevf_dev_uninit(struct rte_eth_dev
>> *eth_dev)
>> static struct eth_driver rte_ixgbe_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_ixgbe_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
>> RTE_PCI_DRV_DETACHABLE,
>> @@ -1580,6 +1584,10 @@ static struct eth_driver rte_ixgbe_pmd = {
>> */
>> static struct eth_driver rte_ixgbevf_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_ixgbevf_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
>> .probe = rte_eth_dev_pci_probe,
>> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
>> index da61a85..e3dcd41 100644
>> --- a/drivers/net/mlx4/mlx4.c
>> +++ b/drivers/net/mlx4/mlx4.c
>> @@ -5907,7 +5907,9 @@ static const struct rte_pci_id mlx4_pci_id_map[]
>> = {
>> static struct eth_driver mlx4_driver = {
>> .pci_drv = {
>> .driver = {
>> - .name = MLX4_DRIVER_NAME
>> + .name = MLX4_DRIVER_NAME,
>> + .probe = rte_eal_pci_probe,
>> + },
>> },
>> .id_table = mlx4_pci_id_map,
>> .probe = mlx4_pci_probe,
>> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
>> index 90cc35e..76dda13 100644
>> --- a/drivers/net/mlx5/mlx5.c
>> +++ b/drivers/net/mlx5/mlx5.c
>> @@ -731,6 +731,7 @@ static struct eth_driver mlx5_driver = {
>> .pci_drv = {
>> .driver = {
>> .name = MLX5_DRIVER_NAME
>> + .probe = rte_eal_pci_probe,
>> },
>> .id_table = mlx5_pci_id_map,
>> .probe = mlx5_pci_probe,
>> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
>> index de80b46..125ba86 100644
>> --- a/drivers/net/nfp/nfp_net.c
>> +++ b/drivers/net/nfp/nfp_net.c
>> @@ -2469,6 +2469,10 @@ static struct rte_pci_id pci_id_nfp_net_map[] = {
>> static struct eth_driver rte_nfp_net_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_nfp_net_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
>> RTE_PCI_DRV_DETACHABLE,
>> diff --git a/drivers/net/qede/qede_ethdev.c
>> b/drivers/net/qede/qede_ethdev.c
>> index d106dd0..31f6733 100644
>> --- a/drivers/net/qede/qede_ethdev.c
>> +++ b/drivers/net/qede/qede_ethdev.c
>> @@ -1642,6 +1642,10 @@ static struct rte_pci_id pci_id_qede_map[] = {
>> static struct eth_driver rte_qedevf_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_qedevf_map,
>> .drv_flags =
>> RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
>> @@ -1655,6 +1659,10 @@ static struct eth_driver rte_qedevf_pmd = {
>> static struct eth_driver rte_qede_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_qede_map,
>> .drv_flags =
>> RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
>> diff --git a/drivers/net/szedata2/rte_eth_szedata2.c
>> b/drivers/net/szedata2/rte_eth_szedata2.c
>> index f3cd52d..a649e60 100644
>> --- a/drivers/net/szedata2/rte_eth_szedata2.c
>> +++ b/drivers/net/szedata2/rte_eth_szedata2.c
>> @@ -1572,6 +1572,10 @@ static const struct rte_pci_id
>> rte_szedata2_pci_id_table[] = {
>> static struct eth_driver szedata2_eth_driver = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = rte_szedata2_pci_id_table,
>> .probe = rte_eth_dev_pci_probe,
>> .remove = rte_eth_dev_pci_remove,
>> diff --git a/drivers/net/thunderx/nicvf_ethdev.c
>> b/drivers/net/thunderx/nicvf_ethdev.c
>> index 466e49c..72ac748 100644
>> --- a/drivers/net/thunderx/nicvf_ethdev.c
>> +++ b/drivers/net/thunderx/nicvf_ethdev.c
>> @@ -2110,6 +2110,10 @@ static const struct rte_pci_id
>> pci_id_nicvf_map[] = {
>> static struct eth_driver rte_nicvf_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_nicvf_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
>> .probe = rte_eth_dev_pci_probe,
>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>> b/drivers/net/virtio/virtio_ethdev.c
>> index 079fd6c..4d5d1bb 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1377,6 +1377,8 @@ static struct eth_driver rte_virtio_pmd = {
>> .pci_drv = {
>> .driver = {
>> .name = "net_virtio",
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> },
>> .id_table = pci_id_virtio_map,
>> .drv_flags = RTE_PCI_DRV_DETACHABLE,
>> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>> b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>> index 8bb13e5..57f66cb 100644
>> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>> @@ -335,6 +335,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
>> static struct eth_driver rte_vmxnet3_pmd = {
>> .pci_drv = {
>> + .driver = {
>> + .probe = rte_eal_pci_probe,
>> + .remove = rte_eal_pci_remove,
>> + },
>> .id_table = pci_id_vmxnet3_map,
>> .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_DETACHABLE,
>> .probe = rte_eth_dev_pci_probe,
>
>
>
^ permalink raw reply
* Re: [PATCH v2 01/12] eal: define container_of macro
From: Shreyansh Jain @ 2016-12-14 5:12 UTC (permalink / raw)
To: Jan Blunck
Cc: dev, David Marchand, Thomas Monjalon, Ferruh Yigit, jianbo.liu,
Jan Viktorin
In-Reply-To: <CALe+Z03RKxZ-CZydfMhZf+geJCOqSCT-3u3jzQi78gnv1FOCtA@mail.gmail.com>
On Wednesday 14 December 2016 03:54 AM, Jan Blunck wrote:
> On Tue, Dec 13, 2016 at 2:37 PM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> From: Jan Blunck <jblunck@infradead.org>
>>
>> This macro is based on Jan Viktorin's original patch but also checks the
>> type of the passed pointer against the type of the member.
>>
>> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
>> [shreyansh.jain@nxp.com: Fix checkpatch error]
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> [jblunck@infradead.org: add type checking and __extension__]
>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>>
>> --
>> v2:
>> - fix checkpatch error
>> ---
>> lib/librte_eal/common/include/rte_common.h | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
>> index db5ac91..3eb8d11 100644
>> --- a/lib/librte_eal/common/include/rte_common.h
>> +++ b/lib/librte_eal/common/include/rte_common.h
>> @@ -331,6 +331,27 @@ rte_bsf32(uint32_t v)
>> #define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)
>> #endif
>>
>> +/**
>> + * Return pointer to the wrapping struct instance.
>> + *
>> + * Example:
>> + *
>> + * struct wrapper {
>> + * ...
>> + * struct child c;
>> + * ...
>> + * };
>> + *
>> + * struct child *x = obtain(...);
>> + * struct wrapper *w = container_of(x, struct wrapper, c);
>> + */
>> +#ifndef container_of
>> +#define container_of(ptr, type, member) (__extension__ ({ \
>> + typeof(((type *)0)->member) * _ptr = (ptr); \
>> + (type *)(((char *)_ptr) - offsetof(type, member));\
>> + }))
>
> This is a checkpatch false positive. It should be fine to ignore this.
> IIRC we already discussed this before.
I too thought something similar was discussed. I tried searching the
archives but couldn't find anything - thus, I thought probably I was
hallucinating :P
So, you want me to revert back the '()' change? Does it impact the
expansion of this macro?
>
>
>> +#endif
>> +
>> #define _RTE_STR(x) #x
>> /** Take a macro value and get a string version of it */
>> #define RTE_STR(x) _RTE_STR(x)
>> --
>> 2.7.4
>>
>
^ permalink raw reply
* Re: [PATCH v2 12/12] drivers: update PMDs to use rte_driver probe and remove
From: Shreyansh Jain @ 2016-12-14 5:14 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Andrew Rybchenko, dev
In-Reply-To: <108fb50f-a5da-1646-5dc4-23b03be6a02e@intel.com>
Hello Ferruh,
On Tuesday 13 December 2016 08:37 PM, Ferruh Yigit wrote:
> On 12/13/2016 1:52 PM, Andrew Rybchenko wrote:
>> On 12/13/2016 04:37 PM, Shreyansh Jain wrote:
>>> These callbacks now act as first layer of PCI interfaces from the Bus.
>>> Bus probe would enter the PMDs through the rte_driver->probe/remove
>>> callbacks, falling to rte_xxx_driver->probe/remove (Currently, all the
>>> drivers are rte_pci_driver).
>>
>> I think similar changes in drivers/net/sfc/sfc_ethdev.c (already in
>> dpdk-next-net) are required as well.
>
> Yes, that change is required, but it is a little tricky because this
> patchset targets main tree where sfc is not merged yet, so this patch
> can't include required patches.
>
> I think it is possible to wait for this patch to be merged into main
> tree, and when next-net rebased on top of it, sfc can be patched
> individually.
>
> So, yes there is a work to do there, but I think it can be postponed a
> little.
>
<snip>
I noticed this email after replying something similar (and a question)
to Andrew. Sorry for double posting same thing.
-
Shreyansh
^ permalink raw reply
* Re: [PATCH v2 3/6] eventdev: implement the northbound APIs
From: Jerin Jacob @ 2016-12-14 6:28 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <20161208095937.GC55440@bricha3-MOBL3.ger.corp.intel.com>
On Thu, Dec 08, 2016 at 09:59:37AM +0000, Bruce Richardson wrote:
> On Wed, Dec 07, 2016 at 10:32:56PM +0530, Jerin Jacob wrote:
> > On Tue, Dec 06, 2016 at 05:17:12PM +0000, Bruce Richardson wrote:
> > > On Tue, Dec 06, 2016 at 09:22:17AM +0530, Jerin Jacob wrote:
> > > > This patch implements northbound eventdev API interface using
> > > > southbond driver interface
> > > >
> > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > ---
> > > > + /* Re allocate memory to store queue priority */
> > > > + queues_prio = dev->data->queues_prio;
> > > > + queues_prio = rte_realloc(queues_prio,
> > > > + sizeof(queues_prio[0]) * nb_queues,
> > > > + RTE_CACHE_LINE_SIZE);
> > > > + if (queues_prio == NULL) {
> > > > + RTE_EDEV_LOG_ERR("failed to realloc queue priority,"
> > > > + " nb_queues %u", nb_queues);
> > > > + return -(ENOMEM);
> > > > + }
> > > > + dev->data->queues_prio = queues_prio;
> > > > +
> > > > + if (nb_queues > old_nb_queues) {
> > > > + uint8_t new_qs = nb_queues - old_nb_queues;
> > > > +
> > > > + memset(queues + old_nb_queues, 0,
> > > > + sizeof(queues[0]) * new_qs);
> > > > + memset(queues_prio + old_nb_queues, 0,
> > > > + sizeof(queues_prio[0]) * new_qs);
> > > > + }
> > > > + } else if (dev->data->queues != NULL && nb_queues == 0) {
> > > > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->queue_release, -ENOTSUP);
> > > > +
> > > > + queues = dev->data->queues;
> > > > + for (i = nb_queues; i < old_nb_queues; i++)
> > > > + (*dev->dev_ops->queue_release)(queues[i]);
> > > > + }
> > > > +
> > > > + dev->data->nb_queues = nb_queues;
> > > > + return 0;
> > > > +}
> > > > +
> > > While the ports array makes sense to have available at the top level of
> > > the API and allocated from rte_eventdev.c, I'm not seeing what the value
> > > of having the queues allocated at that level is. The only time the queue
> > > array is indexed by eventdev layer is when releasing a queue. Therefore,
> > > I suggest just saving the number of queues for sanity checking and let
> > > the queue array allocation and freeing be handled entirely in the
> > > drivers themselves.
> >
> > I thought it would be useful for other drivers. I agree, If something is not
> > common across all the driver lets remove it from common code.
> > I will remove it in v3
> >
> It's not a big deal for us - just an extra assignment we need to do in
> our code path, so if it provides benefit for your driver, leave it in. I
We don't use it either. I will remove it in v3
> just found it strange that that array was never really used by the
> eventdev APIs, which is why I thought it might be better as internal
> only.
>
> /Bruce
^ permalink raw reply
* [PATCH v3] net/ixgbe:fix max packet length in ixgbevf
From: Yi Zhang @ 2016-12-14 18:50 UTC (permalink / raw)
To: maintainer; +Cc: dev, Yi Zhang
Current ixgbevf driver get max_rx_pktlen = 15872, but in fact PF
supports 15872-byte jumbo frame and VF only supports 9728-byte jumbo
frame. If VF is running DPDK driver and set frame_size > 9728 ,PF
running kernel ixgbe driver will report an error and set VF failed.
This patch fixs DPDK ixgbevf driver to get correct jumbo frame size
of VF.
Signed-off-by: Yi Zhang <zhang.yi75@zte.com.cn>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index edc9b22..573252c 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -3168,7 +3168,7 @@ ixgbevf_dev_info_get(struct rte_eth_dev *dev,
dev_info->max_rx_queues = (uint16_t)hw->mac.max_rx_queues;
dev_info->max_tx_queues = (uint16_t)hw->mac.max_tx_queues;
dev_info->min_rx_bufsize = 1024; /* cf BSIZEPACKET in SRRCTL reg */
- dev_info->max_rx_pktlen = 15872; /* includes CRC, cf MAXFRS reg */
+ dev_info->max_rx_pktlen = 9728; /* includes CRC, cf MAXFRS reg */
dev_info->max_mac_addrs = hw->mac.num_rar_entries;
dev_info->max_hash_mac_addrs = IXGBE_VMDQ_NUM_UC_MAC;
dev_info->max_vfs = dev->pci_dev->max_vfs;
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Jerin Jacob @ 2016-12-14 6:40 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <20161208095752.GB55440@bricha3-MOBL3.ger.corp.intel.com>
On Thu, Dec 08, 2016 at 09:57:52AM +0000, Bruce Richardson wrote:
> On Thu, Dec 08, 2016 at 07:18:01AM +0530, Jerin Jacob wrote:
> > On Wed, Dec 07, 2016 at 11:12:51AM +0000, Bruce Richardson wrote:
> > > On Tue, Dec 06, 2016 at 09:22:15AM +0530, Jerin Jacob wrote:
> > > > + */
> > > > +int
> > > > +rte_event_port_link(uint8_t dev_id, uint8_t port_id,
> > > > + const struct rte_event_queue_link link[],
> > > > + uint16_t nb_links);
> > > > +
> > >
> > > Hi again Jerin,
> > >
> > > another small suggestion here. I'm not a big fan of using small
> > > structures to pass parameters into functions, especially when not all
> > > fields are always going to be used. Rather than use the event queue link
> > > structure, can we just pass in two array parameters here - the list of
> > > QIDs, and the list of priorities. In cases where the eventdev
> > > implementation does not support link prioritization, or where the app
> > > does not want different priority mappings , then the second
> > > array can be null [implying NORMAL priority for the don't care case].
> > >
> > > int
> > > rte_event_port_link(uint8_t dev_id, uint8_t port_id,
> > > const uint8_t queues[], const uint8_t priorities[],
> > > uint16_t nb_queues);
> > >
> > > This just makes mapping an array of queues easier, as we can just pass
> > > an array of ints directly in, and it especially makes it easier to
> > > create a single link via:
> > >
> > > rte_event_port_link(dev_id, port_id, &queue_id, NULL, 1);
> >
> > The reason why I thought of creating "struct rte_event_queue_link",
> > - Its easy to add new parameter in link attributes if required
>
> Make the priority value be in a struct, perhaps. That would allow for
> future expansion, while still making it easier for the case where people
> just want the mappings without any prioritization.
OK. I will change the API prototype to align with your proposal in v3
int
rte_event_port_link(uint8_t dev_id, uint8_t port_id,
const uint8_t queues[], const uint8_t priorities[],
uint16_t nb_links);
int
rte_event_port_links_get(uint8_t dev_id, uint8_t port_id,
uint8_t queues[], uint8_t priorities[]);
>
> > - Its _easy_ to implement PAUSE and RESUME in application
> >
> > PAUSE:
> > nr_links = rte_event_port_links_get(,,link)
> > rte_event_port_unlink_all
> >
> > RESUME:
> > rte_event_port_link(,,link, nr_links);
>
> Ok, I had missed that implication. Since that is probably an important
> operation we might want to do, perhaps links_get API should be updated
> too to keep parameter matching.
>
> /Bruce
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Jerin Jacob @ 2016-12-14 6:55 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <20161209151142.GA14536@bricha3-MOBL3.ger.corp.intel.com>
On Fri, Dec 09, 2016 at 03:11:42PM +0000, Bruce Richardson wrote:
> On Fri, Dec 09, 2016 at 02:11:15AM +0530, Jerin Jacob wrote:
> > On Thu, Dec 08, 2016 at 09:30:49AM +0000, Bruce Richardson wrote:
> > > On Thu, Dec 08, 2016 at 12:23:03AM +0530, Jerin Jacob wrote:
> > > > On Tue, Dec 06, 2016 at 04:51:19PM +0000, Bruce Richardson wrote:
> > > > > On Tue, Dec 06, 2016 at 09:22:15AM +0530, Jerin Jacob wrote:
> > > > > I think this might need to be clarified. The device doesn't need to be
> > > > > reconfigured, but does it need to be stopped? In SW implementation, this
> > > > > affects how much we have to make things thread-safe. At minimum I think
> > > > > we should limit this to having only one thread call the function at a
> > > > > time, but we may allow enqueue dequeue ops from the data plane to run
> > > > > in parallel.
> > > >
> > > > Cavium implementation can change it at runtime without re-configuring or stopping
> > > > the device to support runtime load balancing from the application perspective.
> > > >
> > > > AFAIK, link establishment is _NOT_ fast path API. But the application
> > > > can invoke it from worker thread whenever there is a need for re-wiring
> > > > the queue to port connection for better explicit load balancing. IMO, A
> > > > software implementation with lock is fine here as we don't use this in
> > > > fastpath.
> > > >
> > > > Thoughts?
> > > > >
> > >
> > > I agree that it's obviously not fast-path. Therefore I suggest that we
> > > document that this API should be safe to call while the data path is in
> > > operation, but that it should not be called by multiple cores
> > > simultaneously i.e. single-writer, multi-reader safe, but not
> > > multi-writer safe. Does that seem reasonable to you?
> >
> > If I understand it correctly, per "event port" their will be ONLY ONE
> > writer at time.
> >
> > i.e, In the valid case, Following two can be invoked in parallel
> > rte_event_port_link(dev_id, 0 /*port_id*/,..)
> > rte_event_port_link(dev_id, 1 /*port_id*/,..)
> >
> > But, not invoking rte_event_port_link() on the _same_ event port in parallel
> >
> > Are we on same page?
> >
> > Jerin
> >
> Not entirely. Since our current software implementation pushes the events
> from the internal queues to the ports, rather than having the ports pull
> the events, the links are tracked at the qid level rather than at the
> port one. So having two link operations on two separate ports at the
> same time could actually conflict for us, because they attempt to modify
> the mappings for the same queue. That's why for us the number of
> simultaneous link calls is important.
> However, given that this is not fast-path, we can probably work around
> this with locking internally. The main ask is that we explicitly
Yes, It is in slow-path. IMO, no harm in adding the lock internally and it
helps the application too.
> document what are the expected safe and unsafe conditions under which
> this call can be made.
As we agreed and it is a norm in DPDK that operation on same queue id
(in our case same port id) _cannot_ not be invoked in parallel.
Apart from the above constrain, Let us know what are other constrains you
want to add(if any).
>
> /Bruce
^ permalink raw reply
* Re: [PATCH v3] net/ixgbe:fix max packet length in ixgbevf
From: Yuanhan Liu @ 2016-12-14 7:06 UTC (permalink / raw)
To: Yi Zhang; +Cc: Helin Zhang, Konstantin Ananyev, dev
In-Reply-To: <20161214185019.19581-1-zhang.yi75@zte.com.cn>
On Thu, Dec 15, 2016 at 02:50:19AM +0800, Yi Zhang wrote:
> Current ixgbevf driver get max_rx_pktlen = 15872, but in fact PF
> supports 15872-byte jumbo frame and VF only supports 9728-byte jumbo
> frame. If VF is running DPDK driver and set frame_size > 9728 ,PF
> running kernel ixgbe driver will report an error and set VF failed.
> This patch fixs DPDK ixgbevf driver to get correct jumbo frame size
> of VF.
>
> Signed-off-by: Yi Zhang <zhang.yi75@zte.com.cn>
Hi,
I saw you have sent this patch 3 times; are you looking for review?
If so, you should not resend it many times, instead, you could reply
to your patch if it hasn't got any comments after one week, with
something like "some one can help review this?", or even a simple
"ping ..." might just work.
Besides that, you should Cc the corresponding maintainers, but not
"maintainer@some.org", which is just an example: I doubt such email
exists.
You could get the maintainers from the MAINTAINERS. Take ixgbe as
example, it's:
Intel ixgbe
M: Helin Zhang <helin.zhang@intel.com>
M: Konstantin Ananyev <konstantin.ananyev@intel.com>
F: drivers/net/ixgbe/
F: doc/guides/nics/ixgbe.rst
F: doc/guides/nics/intel_vf.rst
So for this patch, you should at least Cc (or To) Helin and Konstantin.
I have done that for you this time.
--yliu
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index edc9b22..573252c 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -3168,7 +3168,7 @@ ixgbevf_dev_info_get(struct rte_eth_dev *dev,
> dev_info->max_rx_queues = (uint16_t)hw->mac.max_rx_queues;
> dev_info->max_tx_queues = (uint16_t)hw->mac.max_tx_queues;
> dev_info->min_rx_bufsize = 1024; /* cf BSIZEPACKET in SRRCTL reg */
> - dev_info->max_rx_pktlen = 15872; /* includes CRC, cf MAXFRS reg */
> + dev_info->max_rx_pktlen = 9728; /* includes CRC, cf MAXFRS reg */
> dev_info->max_mac_addrs = hw->mac.num_rar_entries;
> dev_info->max_hash_mac_addrs = IXGBE_VMDQ_NUM_UC_MAC;
> dev_info->max_vfs = dev->pci_dev->max_vfs;
> --
> 2.9.3
>
^ permalink raw reply
* Re: [PATCH 5/5] net/virtio: fix Tso when mbuf is shared
From: Yuanhan Liu @ 2016-12-14 7:27 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev, maxime.coquelin, huawei.xie, stephen
In-Reply-To: <1479977798-13417-6-git-send-email-olivier.matz@6wind.com>
Hi Olivier,
Firstly sorry for late response!
On Thu, Nov 24, 2016 at 09:56:38AM +0100, Olivier Matz wrote:
> With virtio, doing tso requires to modify the network
> packet data:
I thought more about it this time, and I'm wondering why it's needed.
> - the dpdk API requires to set the l4 checksum to an
> Intel-Nic-like pseudo header checksum that does
> not include the ip length
If the packet is for a NIC pmd driver in the end, then the NIC driver
(or application) would handle the checksum correctly. You could check
the tx_prep patchset for example.
> - the virtio peer expects that the l4 checksum is
> a standard pseudo header checksum.
For this case, the checksum is then not needed: we could assume the data
between virtio to virtio transmission on the same host is always valid,
that checksum validation is unnecessary.
So, in either case, it doesn't seem to me we have to generate the checksum
here. Or am I miss something?
OTOH, even if it does, I still see some issues (see below).
> /* TCP Segmentation Offload */
> if (cookie->ol_flags & PKT_TX_TCP_SEG) {
> - virtio_tso_fix_cksum(cookie);
> + offset = virtio_tso_fix_cksum(cookie,
> + RTE_PTR_ADD(hdr, start_dp[hdr_idx].len),
> + VIRTIO_MAX_HDR_SZ);
> + if (offset > 0) {
> + RTE_ASSERT(can_push != 0);
I think it's (can_push == 0) ?
> + start_dp[hdr_idx].len += offset;
Actually, there is an assumption if you do this, that the backend driver
must have to support ANY_LAYOUT. Otherwise, it won't work: the driver
would expect the header and packet data is totally separated into two
desc buffers.
Though the assumption is most likely true in nowadays, I don't think
it's a guarantee.
--yliu
^ permalink raw reply
* Re: [RFC PATCH] eventdev: add buffered enqueue and flush APIs
From: Jerin Jacob @ 2016-12-14 7:44 UTC (permalink / raw)
To: Eads, Gage
Cc: dev@dpdk.org, Richardson, Bruce, Van Haaren, Harry,
hemant.agrawal@nxp.com
In-Reply-To: <9184057F7FC11744A2107296B6B8EB1E01E38655@FMSMSX108.amr.corp.intel.com>
On Mon, Dec 12, 2016 at 05:56:32PM +0000, Eads, Gage wrote:
>
>
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Wednesday, December 7, 2016 10:42 PM
> > To: Eads, Gage <gage.eads@intel.com>
> > 1) What if the burst has ATOMIC flows and if we are NOT en-queuing to the
> > implementation then other event ports won't get the packets from the same
> > ATOMIC tag ? BAD. Right?
>
> I'm not sure what scenario you're describing here. The buffered (as implemented in my patch) and non-buffered enqueue operations are functionally the same (as long as the buffer is flushed), the difference lies in when the events are moved from the application level to the PMD.
OK. I will try to explain with time-line
Assume,
flush size: 16
bust size: 4
At t0: dequeued 4 events(3 ordered and 1 atomic events)
At t1: after processing the events, store to the events local buffer
At t2: request to dequeue 4 more events
Now, Since scheduler has been scheduled an atomic event to port at t0, it
can not schedule an atomic event of same TAG to _any port_. As atomic
events from the same TAG's in-flight entry will be always one to enable
the critical section processing in the packet flow.
>
> > 2) At least, In our HW implementation, The event buffer strategy is more like, if
> > you enqueue to HW then ONLY you get the events from dequeue provided if op
> > == RTE_EVENT_OP_FORWARD.So it will create deadlock.i.e application cannot
> > hold the events with RTE_EVENT_OP_FORWARD
>
> If I'm reading this correctly, you're concerned that buffered events can result in deadlock if they're not flushed. Whether the buffering is done in the app itself, inline in the API, or in the PMDs, not flushing the buffer is an application bug. E.g. the app could be fixed by flushing its enqueue buffer after processing every burst dequeued event set, or only if dequeue returns 0 events.
No. At least, our HW implementation, it maintains the state of scheduled events
to a port. Drivers get next set of events ONLY if driver submits
the events which already got on the first dequeue.i.e application cannot
hold the events with RTE_EVENT_OP_FORWARD
>
> > 3) So considering the above case there is nothing like flush for us
> > 4) In real high throughput benchmark case, we will get the packets at the rate
> > of max burst and then we always needs to memcpy before we flush.
> > Otherwise there will be ordering issue as burst can get us the packet from
> > different flows(unlike polling mode)
>
> I take it you're referring to the memcpy in the patch, and not an additional memcpy? At any rate, I'm hoping that SIMD instructions can optimize the 16B event copy.
Hmm. The point was we need to memcpy all the time to maintain the order.
>
> >
> > >
> > > > and some does not need to hold the buffers if it is DDR backed.
> > >
> >
> > See above. I am not against burst processing in "application".
> > The flush does not make sense for us in HW perspective and it is costly for us if
> > we trying generalize it.
> >
>
> Besides the data copy that buffering requires, are there additional costs from your perspective?
It won't even work in our case as HW maintains the context on dequeued
events.
I suggest checking the function call overhead. If it turned out
to have the impact on the performance.Then we can split flow based on capability
flag but I recommend it as last option.
>
> > >
> > > I'm skeptical that other buffering strategies would emerge, but I can only
> > speculate on Cavium/NXP/etc. NPU software.
> > i>
> > > > IHMO, This may not be the candidate for common code. I guess you can
> > > > move this to driver side and abstract under SW driver's enqueue_burst.
> > > >
> > >
> > > I don't think that will work without adding a flush API, otherwise we could
> > have indefinitely buffered events. I see three ways forward:
> >
> > I agree. More portable way is to move the "flush" to the implementation and
> > "flush"
> > whenever it makes sense to PMD.
> >
> > >
> > > - The proposed approach
> > > - Add the proposed functions but make them implementation-specific.
> > > - Require the application to write its own buffering logic (i.e. no
> > > API change)
> >
> > I think, If the additional function call overhead cost is too much for SW
> > implementation then we can think of implementation-specific API or custom
> > application flow based on SW driver.
> >
> > But I am not fan of that(but tempted do now a days), If we take that route, we
> > have truckload of custom implementation specific API and now we try to hide
> > all black magic under enqueue/dequeue to make it portable at some expense.
>
> Agreed, it's not worth special-casing the API with this relatively minor addition.
>
> Thanks,
> Gage
^ permalink raw reply
* Re: [RFC PATCH] eventdev: add buffered enqueue and flush APIs
From: Jerin Jacob @ 2016-12-14 7:52 UTC (permalink / raw)
To: Eads, Gage
Cc: dev@dpdk.org, Richardson, Bruce, Van Haaren, Harry,
hemant.agrawal@nxp.com
In-Reply-To: <9184057F7FC11744A2107296B6B8EB1E01E38655@FMSMSX108.amr.corp.intel.com>
On Mon, Dec 12, 2016 at 05:56:32PM +0000, Eads, Gage wrote:
>
>
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Wednesday, December 7, 2016 10:42 PM
> > To: Eads, Gage <gage.eads@intel.com>
> > Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Van
> > Haaren, Harry <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com
> > Subject: Re: [RFC PATCH] eventdev: add buffered enqueue and flush APIs
> >
> > On Mon, Dec 05, 2016 at 11:30:46PM +0000, Eads, Gage wrote:
> > >
> > > > On Dec 3, 2016, at 5:18 AM, Jerin Jacob
> > <jerin.jacob@caviumnetworks.com> wrote:
> > > >
> > > >> On Fri, Dec 02, 2016 at 01:45:56PM -0600, Gage Eads wrote:
> > > >> This commit adds buffered enqueue functionality to the eventdev API.
> > > >> It is conceptually similar to the ethdev API's tx buffering,
> > > >> however with a smaller API surface and no dropping of events.
> > > >
> > > > Hello Gage,
> > > > Different implementation may have different strategies to hold the buffers.
> > >
> > > A benefit of inlining the buffering logic in the header is that we avoid the
> > overhead of entering the PMD for what is a fairly simple operation (common
> > case: add event to an array, increment counter). If we make this
> > implementation-defined (i.e. use PMD callbacks), we lose that benefit.
> > In general, I agree from the system perspective. But, few general issues with
> > eventdev integration part,
> >
> > 1) What if the burst has ATOMIC flows and if we are NOT en-queuing to the
> > implementation then other event ports won't get the packets from the same
> > ATOMIC tag ? BAD. Right?
>
> I'm not sure what scenario you're describing here. The buffered (as implemented in my patch) and non-buffered enqueue operations are functionally the same (as long as the buffer is flushed), the difference lies in when the events are moved from the application level to the PMD.
Another point, we treat dequeue as _implicit_ event release of the
existing dequeued events, so with proposed buffering scheme breaks all the logic.
^ permalink raw reply
* fm10k
From: Ruth Christen @ 2016-12-14 8:41 UTC (permalink / raw)
To: dev@dpdk.org
Hey guys,
I'm using dpdk 16.4 and fm10k card. According to the code, there's no support for disabling vlan stripping and VLAN QinQ in pmd fm10k.
Does anybody know why? If there's any way to work-around it, or when is a behavior change expected?
I need my VF to receive the packets with the VLAN headers.
Even if it's possible to change this configurations through the linux kernel fm10k driver?
Thanks!
^ permalink raw reply
* Re: [PATCH v2 12/12] drivers: update PMDs to use rte_driver probe and remove
From: Shreyansh Jain @ 2016-12-14 9:49 UTC (permalink / raw)
To: dev, Jan Blunck; +Cc: david.marchand, thomas.monjalon, ferruh.yigit, jianbo.liu
In-Reply-To: <1481636232-2300-13-git-send-email-shreyansh.jain@nxp.com>
On Tuesday 13 December 2016 07:07 PM, Shreyansh Jain wrote:
> These callbacks now act as first layer of PCI interfaces from the Bus.
> Bus probe would enter the PMDs through the rte_driver->probe/remove
> callbacks, falling to rte_xxx_driver->probe/remove (Currently, all the
> drivers are rte_pci_driver).
>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
> drivers/net/bnx2x/bnx2x_ethdev.c | 8 ++++++++
> drivers/net/bnxt/bnxt_ethdev.c | 4 ++++
> drivers/net/cxgbe/cxgbe_ethdev.c | 4 ++++
> drivers/net/e1000/em_ethdev.c | 4 ++++
> drivers/net/e1000/igb_ethdev.c | 8 ++++++++
> drivers/net/ena/ena_ethdev.c | 4 ++++
> drivers/net/enic/enic_ethdev.c | 4 ++++
> drivers/net/fm10k/fm10k_ethdev.c | 4 ++++
> drivers/net/i40e/i40e_ethdev.c | 4 ++++
> drivers/net/i40e/i40e_ethdev_vf.c | 4 ++++
> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++++++
> drivers/net/mlx4/mlx4.c | 4 +++-
> drivers/net/mlx5/mlx5.c | 1 +
> drivers/net/nfp/nfp_net.c | 4 ++++
> drivers/net/qede/qede_ethdev.c | 8 ++++++++
> drivers/net/szedata2/rte_eth_szedata2.c | 4 ++++
> drivers/net/thunderx/nicvf_ethdev.c | 4 ++++
> drivers/net/virtio/virtio_ethdev.c | 2 ++
> drivers/net/vmxnet3/vmxnet3_ethdev.c | 4 ++++
> 19 files changed, 86 insertions(+), 1 deletion(-)
>
<snip>
drivers/crypto/qat/rte_qat_cryptodev.c should also be changed for this.
It seems to be only PCI registered PMD. All others are VDEV.
I will send a v3 soon to fix this.
@Jan, would you be looking in the VDEV part or should I start with that? [1]
[1] http://dpdk.org/ml/archives/dev/2016-November/050443.html
^ permalink raw reply
* Re: [PATCH 00/28] introduce I/O device memory read/write operations
From: Bruce Richardson @ 2016-12-14 10:12 UTC (permalink / raw)
To: Yuanhan Liu
Cc: Jerin Jacob, dev, konstantin.ananyev, thomas.monjalon, jianbo.liu,
viktorin
In-Reply-To: <20161214025357.GF18991@yliu-dev.sh.intel.com>
On Wed, Dec 14, 2016 at 10:53:57AM +0800, Yuanhan Liu wrote:
> On Wed, Dec 14, 2016 at 07:25:30AM +0530, Jerin Jacob wrote:
> > patchset 14-28: Replace the raw readl/writel in the drivers with
> > new rte_read[b/w/l/q], rte_write[b/w/l/q] eal abstraction
>
> Instead of rte_read[b/w/l/q], there is another typical naming style:
> rte_read[8/16/32/64]. Any preferences? If you ask me, I'd prefer the
> later.
>
I think I prefer the latter too, as it aligns with our naming of atomic
functions and our use of uint16_t etc. types.
/Bruce
^ permalink raw reply
* Re: [PATCH 01/22] ethdev: introduce generic flow API
From: Kevin Traynor @ 2016-12-14 11:48 UTC (permalink / raw)
To: Adrien Mazarguil
Cc: dev, Thomas Monjalon, Pablo de Lara, Olivier Matz,
sugesh.chandran
In-Reply-To: <20161208170715.GM10340@6wind.com>
hi Adrien, sorry for the delay
<...>
>>>>
>>>> Is it expected that the application or pmd will provide locking between
>>>> these functions if required? I think it's going to have to be the app.
>>>
>>> Locking is indeed expected to be performed by applications. This API only
>>> documents places where locking would make sense if necessary and expected
>>> behavior.
>>>
>>> Like all control path APIs, this one assumes a single control thread.
>>> Applications must take the necessary precautions.
>>
>> If you look at OVS now it's quite possible that you have 2 rx queues
>> serviced by different threads, that would also install the flow rules in
>> the software flow caches - possibly that could extend to adding hardware
>> flows. There could also be another thread that is querying for stats. So
>> anything that can be done to minimise the locking would be helpful -
>> maybe query() could be atomic and not require any locking?
>
> I think we need basic functions with as few constraints as possible on PMDs
> first, this API being somewhat complex to implement on their side. That
> covers the common use case where applications have a single control thread
> or otherwise perform locking on their own.
>
> Once the basics are there for most PMDs, we may add new functions, items,
> properties and actions that provide additional constraints (timing,
> multi-threading and so on), which remain to be defined according to
> feedback. It is designed to be extended without causing ABI breakage.
I think Sugesh and I are trying to foresee some of the issues that may
arise when integrating with something like OVS. OTOH it's
hard/impossible to say what will be needed exactly in the API right now
to make it suitable for OVS.
So, I'm ok with the approach you are taking by exposing a basic API
but I think there should be an expectation that it may not be sufficient
for a project like OVS to integrate in and may take several
iterations/extensions - don't go anywhere!
>
> As for query(), let's see how PMDs handle it first. A race between query()
> and create() on a given device is almost unavoidable without locking, same
> for queries that reset counters in a given flow rule. Basic parallel queries
> should not cause any harm otherwise, although this cannot be guaranteed yet.
You still have a race if there is locking, except it is for the lock,
but it has the same effect. The downside of my suggestion is that all
the PMDs would need to guarantee they could gets stats atomically - I'm
not sure if they can or it's too restrictive.
>
<...>
>>
>>>
>>>>> +
>>>>> +/**
>>>>> + * Destroy a flow rule on a given port.
>>>>> + *
>>>>> + * Failure to destroy a flow rule handle may occur when other flow rules
>>>>> + * depend on it, and destroying it would result in an inconsistent state.
>>>>> + *
>>>>> + * This function is only guaranteed to succeed if handles are destroyed in
>>>>> + * reverse order of their creation.
>>>>
>>>> How can the application find this information out on error?
>>>
>>> Without maintaining a list, they cannot. The specified case is the only
>>> possible guarantee. That does not mean PMDs should not do their best to
>>> destroy flow rules, only that ordering must remain consistent in case of
>>> inability to destroy one.
>>>
>>> What do you suggest?
>>
>> I think if the app cannot remove a specific rule it may want to remove
>> all rules and deal with flows in software for a time. So once the app
>> knows it fails that should be enough.
>
> OK, then since destruction may return an error already, is it fine?
> Applications may call rte_flow_flush() (not supposed to fail unless there is
> a serious issue, abort() in that case) and switch to SW fallback.
yes, it's fine.
>
<...>
>>>>> + * @param[out] error
>>>>> + * Perform verbose error reporting if not NULL.
>>>>> + *
>>>>> + * @return
>>>>> + * 0 on success, a negative errno value otherwise and rte_errno is set.
>>>>> + */
>>>>> +int
>>>>> +rte_flow_query(uint8_t port_id,
>>>>> + struct rte_flow *flow,
>>>>> + enum rte_flow_action_type action,
>>>>> + void *data,
>>>>> + struct rte_flow_error *error);
>>>>> +
>>>>> +#ifdef __cplusplus
>>>>> +}
>>>>> +#endif
>>>>
>>>> I don't see a way to dump all the rules for a port out. I think this is
>>>> neccessary for degbugging. You could have a look through dpif.h in OVS
>>>> and see how dpif_flow_dump_next() is used, it might be a good reference.
>>>
>>> DPDK does not maintain flow rules and, depending on hardware capabilities
>>> and level of compliance, PMDs do not necessarily do it either, particularly
>>> since it requires space and application probably have a better method to
>>> store these pointers for their own needs.
>>
>> understood
>>
>>>
>>> What you see here is only a PMD interface. Depending on applications needs,
>>> generic helper functions built on top of these may be added to manage flow
>>> rules in the future.
>>
>> I'm thinking of the case where something goes wrong and I want to get a
>> dump of all the flow rules from hardware, not query the rules I think I
>> have. I don't see a way to do it or something to build a helper on top of?
>
> Generic helper functions would exist on top of this API and would likely
> maintain a list of flow rules themselves. The dump in that case would be
> entirely implemented in software. I think that recovering flow rules from HW
> may be complicated in many cases (even without taking storage allocation and
> rules conversion issues into account), therefore if there is really a need
> for it, we could perhaps add a dump() function that PMDs are free to
> implement later.
>
ok. Maybe there are some more generic stats that can be got from the
hardware that would help debugging that would suffice, like total flow
rule hits/misses (i.e. not on a per flow rule basis).
You can get this from the software flow caches and it's widely used for
debugging. e.g.
pmd thread numa_id 0 core_id 3:
emc hits:0
megaflow hits:0
avg. subtable lookups per hit:0.00
miss:0
^ permalink raw reply
* Re: [PATCH 04/13] acl: allow zero verdict
From: Ananyev, Konstantin @ 2016-12-14 12:16 UTC (permalink / raw)
To: Michal Miroslaw; +Cc: dev@dpdk.org
In-Reply-To: <20161214021111.2l4h4gsbjxmbffca@rere.qmqm.pl>
Hi Michal,
> -----Original Message-----
> From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> Sent: Wednesday, December 14, 2016 2:11 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
>
> On Tue, Dec 13, 2016 at 09:55:12PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> > > Sent: Tuesday, December 13, 2016 6:03 PM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
> > >
> > > On Tue, Dec 13, 2016 at 05:27:31PM +0000, Ananyev, Konstantin wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> > > > > Sent: Tuesday, December 13, 2016 4:14 PM
> > > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > > > Cc: dev@dpdk.org
> > > > > Subject: Re: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
> > > > >
> > > > > On Tue, Dec 13, 2016 at 03:13:42PM +0000, Ananyev, Konstantin wrote:
> > > > > [...]
> > > > > > > > > > > Subject: [dpdk-dev] [PATCH 04/13] acl: allow zero verdict
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
> > > > > > > > > > > ---
> > > > > > > > > > > lib/librte_acl/rte_acl.c | 3 +--
> > > > > > > > > > > lib/librte_acl/rte_acl.h | 2 --
> > > > > > > > > > > lib/librte_table/rte_table_acl.c | 2 +-
> > > > > > > > > > > 3 files changed, 2 insertions(+), 5 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/lib/librte_acl/rte_acl.c b/lib/librte_acl/rte_acl.c
> > > > > > > > > > > index 8b7e92c..d1f40be 100644
> > > > > > > > > > > --- a/lib/librte_acl/rte_acl.c
> > > > > > > > > > > +++ b/lib/librte_acl/rte_acl.c
> > > > > > > > > > > @@ -313,8 +313,7 @@ acl_check_rule(const struct rte_acl_rule_data *rd)
> > > > > > > > > > > if ((RTE_LEN2MASK(RTE_ACL_MAX_CATEGORIES, typeof(rd->category_mask)) &
> > > > > > > > > > > rd->category_mask) == 0 ||
> > > > > > > > > > > rd->priority > RTE_ACL_MAX_PRIORITY ||
> > > > > > > > > > > - rd->priority < RTE_ACL_MIN_PRIORITY ||
> > > > > > > > > > > - rd->userdata == RTE_ACL_INVALID_USERDATA)
> > > > > > > > > > > + rd->priority < RTE_ACL_MIN_PRIORITY)
> > > > > > > > > > > return -EINVAL;
> > > > > > > > > > > return 0;
> > > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > I am not sure, how it supposed to work properly?
> > > > > > > > > > Zero value is reserved and ifnicates that no match were found for that input.
> > > > > > > > >
> > > > > > > > > This is actually in use by us. In our use we don't need to differentiate
> > > > > > > > > matching a rule with zero verdict vs not matching a rule at all. I also
> > > > > > > > > have a patch that changes the value returned in non-matching case, but
> > > > > > > > > it's in "dirty hack" state, as of yet.
> > > > > > > >
> > > > > > > > With that chane rte_acl_classify() might produce invalid results.
> > > > > > > > Even if you don't need it (I still don't understand how) , it doesn't mean other people
> > > > > > > > don't need it either and it is ok to change it.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > The ACL code does not treat zero userdata specially, so this is only
> > > > > > > > > a policy choice and as such would be better to be made by the user.
> > > > > > > >
> > > > > > > > I believe it does.
> > > > > > > > userdata==0 is a reserved value.
> > > > > > > > When rte_acl_clasify() returns 0 for that particular input, it means 'no matches were found'.
> > > > > > >
> > > > > > > Dear Konstantin,
> > > > > > >
> > > > > > > Can you describe how the ACL code treats zero specially? I could not find
> > > > > > > anything, really. The only thing I found is that iff I use zero userdata
> > > > > > > in a rule I won't be able to differentiate a case where it matched from
> > > > > > > a case where no rule matched.
> > > > > >
> > > > > > Yes, that's what I am talking about.
> > > > > >
> > > > > > > If I all my rules have non-zero userdata,
> > > > > > > then this patch changes nothing.
> > > > > >
> > > > > > Ok, then why do you remove a code that does checking for invalid userdata==0?
> > > > > > That supposed to prevent user to setup invalid value by mistake.
> > > > > >
> > > > > > But if I have a table where 0 means drop
> > > > > > > (default-drop policy) then being able to use zero userdata in DROP rules
> > > > > > > makes the ACLs just that more useful.
> > > > > >
> > > > > > Ok, and what prevents you from do +1 to your policy values before
> > > > > > you insert it into the ACL table and -1 after you retrieved it via rte_acl_classify()?
> > > > >
> > > > > The check is enforcing an assumption that all users want to distinguish
> > > > > the cases whether any rule matched and whether no rules matched. Not all
> > > > > users do, hence the assumption is invalid and this patch removes it.
> > > >
> > > > The check is based on the assumption that users might need to distinguish
> > > > the situation when no rules were matched.
> > > > To support that we need a reserved userdata value, which would mean
> > > > NO_MATCH.
> > > > From what I heard, most users do need this ability, those who don't
> > > > can easily overcome it.
> > >
> > > That's actually my point. Some users need the distinction, so they don't use
> > > zero userdata in their rules and have their work done. Some users don't need
> > > it and would prefer to just use the convenience of zero being no-match signal
> > > to insert "non-matching" rules (now they have to check two values for the
> > > same signal).
> > >
> > > > > Yes, people can work around it by loosing 1 of 2^32 useful values and
> > > > > convoluting their code.
> > > > Yes, one of 2^32 values is reserved.
> > > > Any reason why (2^32 - 1) values might not be enough?
> > >
> > > Sure. We're using userdata as a bitmask of actions to take on the packet,
> > > and because of this restriction we're loosing half of the userdata field.
> > > If we would add this "decrement if non-zero" workaround this would keep
> > > biting us on every occasion where we touch the ACL verdict code.
> > >
> > > > > You seem to argue that 0 is somehow an invalid value, but I can't find
> > > > > anything in the ACL that would require it to be so. Could you point me
> > > > > to the code in DPDK where this actually matters?
> > > >
> > > > It was a while, when I looked into ACL code in details, but as remember
> > > > that's the only reason: we need some value to be reserved as NO_MATCH.
> > > > Let say in build_trie() we set results to zero for rules with unused categories:
> > > > for (m = context->cfg.num_categories; 0 != m--; ) {
> > > > if (rule->f->data.category_mask & (1 << m)) {
> > > > end->mrt->results[m] = rule->f->data.userdata;
> > > > end->mrt->priority[m] = rule->f->data.priority;
> > > > } else {
> > > > end->mrt->results[m] = 0;
> > > > end->mrt->priority[m] = 0;
> > > > }
> > > > }
> > >
> > > So, if I understand correctly, 0 is a default value for category result.
> > > Any matching rule with priority >= 0 will override it (leaving last highest
> > > priority rule's userdata). This will just work the same for anyone needing
> > > the distinction (when he doesn't use userdata == 0) and also for those who
> > > don't -- when the restriction is removed.
> > >
> > > I think that it comes to documenting the behaviour and let users choose
> > > their way. At the beginning I haven't found any mention of the restriction
> > > in the docs, so I had to spend a fair amount of time to find out why the
> > > zero is so special (it wasn't).
> >
> > Ok, so you suggest the following:
> > 1. Zero value for both userdata and results still has a special meaning: NO_MATCH.
> > 2. Allow user to create a rule(s) that would on hit return NO_MATCH for it,
> > as if no rule was matched by that input (i.e. rule's userdata==0).
> > Is my understanding correct?
>
> That is exactly it.
Ok, the idea still seems a bit unusual to me, but I suppose might be useful for some cases.
Again, can't think up a scenario when it would break something.
So don't have any good reason to object :)
Before we proceed with it can I ask you for two things:
- Verify that autotest_acl works as expected.
- Update PG: http://dpdk.org/doc/guides/prog_guide/packet_classif_access_ctrl.html#rule-definition
I think these lines need to be repharsed because of that patch:
"userdata: A user-defined field that could be any value except zero."
Konstantin
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Jerin Jacob @ 2016-12-14 13:13 UTC (permalink / raw)
To: Van Haaren, Harry
Cc: dev@dpdk.org, thomas.monjalon@6wind.com, Richardson, Bruce,
hemant.agrawal@nxp.com, Eads, Gage
In-Reply-To: <E923DB57A917B54B9182A2E928D00FA6129C9B9C@IRSMSX102.ger.corp.intel.com>
On Thu, Dec 08, 2016 at 11:02:16AM +0000, Van Haaren, Harry wrote:
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Thursday, December 8, 2016 1:24 AM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
>
> <snip>
>
> > > * Operation and sched_type *increased* to 4 bits each (from previous value of 2) to
> > allow future expansion without ABI changes
> >
> > Anyway it will break ABI if we add new operation. I would propose to keep 4bit
> > reserved and add it when required.
>
> Ok sounds good. I'll suggest to move it to the middle between operation or sched type, which would allow expanding operation without ABI breaks. On expanding the field would remain in the same place with the same bits available in that place (no ABI break), but new bits can be added into the currently reserved space.
OK. We will move the rsvd field as you suggested.
>
>
> > > * Restore flow_id to 24 bits of a 32 bit int (previous size was 20 bits)
> > > * sub-event-type reduced to 4 bits (previous value was 8 bits). Can we think of
> > situations where 16 values for application specified identifiers of each event-type is
> > genuinely not enough?
> > One packet will not go beyond 16 stages but an application may have more stages and
> > each packet may go mutually exclusive stages. For example,
> >
> > packet 0: stagex_0 ->stagex_1
> > packet 1: stagey_0 ->stagey_1
> >
> > In that sense, IMO, more than 16 is required.(AFIAK, VPP has any much larger limit on
> > number of stages)
>
> My understanding was that stages are linked to event queues, so the application can determine the stage the packet comes from by reading queue_id?
That is one way of doing it. But it is limited to number of queues
therefore scalability issues.Another approach is through
sub_event_type scheme without depended on the number of queues.
>
> I'm not opposed to having an 8 bit sub_event_type, but it seems unnecessarily large from my point of view. If you have a use for it, I'm ok with 8 bits.
OK
>
>
> > > In my opinion this structure layout is more balanced, and will perform better due to
> > less loads that will need masking to access the required value.
> > OK. Considering more balanced layout and above points. I propose following scheme(based on
> > your input)
> >
> > union {
> > uint64_t event;
> > struct {
> > uint32_t flow_id: 20;
> > uint32_t sub_event_type : 8;
> > uint32_t event_type : 4;
> >
> > uint8_t rsvd: 4; /* for future additions */
> > uint8_t operation : 2; /* new fwd drop */
> > uint8_t sched_type : 2;
> >
> > uint8_t queue_id;
> > uint8_t priority;
> > uint8_t impl_opaque;
> > };
> > };
> >
> > Feedback and improvements welcomed,
>
>
> So incorporating my latest suggestions on moving fields around, excluding sub_event_type *size* changes:
>
> union {
> uint64_t event;
> struct {
> uint32_t flow_id: 20;
> uint32_t event_type : 4;
> uint32_t sub_event_type : 8; /* 8 bits now naturally aligned */
Just one suggestion here. I am not sure about the correct split between
number of bits to represent flow_id and sub_event_type fields. And its
connected in our implementation, so I propose to move sub_event_type up so
that future flow_id/sub_event_type bit field size change request wont
impact our implementation. Since it is represented as 32bit as whole, I
don't think there is an alignment issue.
So incorporating my latest suggestions on moving sub_event_type field around:
union {
uint64_t event;
struct {
uint32_t flow_id: 20;
uint32_t sub_event_type : 8;
uint32_t event_type : 4;
uint8_t operation : 2; /* new fwd drop */
uint8_t rsvd: 4; /* for future additions, can be expanded into without ABI break */
uint8_t sched_type : 2;
uint8_t queue_id;
uint8_t priority;
uint8_t impl_opaque;
};
};
^ permalink raw reply
* Re: [PATCH 00/28] introduce I/O device memory read/write operations
From: Jerin Jacob @ 2016-12-14 13:18 UTC (permalink / raw)
To: Yuanhan Liu
Cc: dev, konstantin.ananyev, thomas.monjalon, bruce.richardson,
jianbo.liu, viktorin
In-Reply-To: <20161214025357.GF18991@yliu-dev.sh.intel.com>
On Wed, Dec 14, 2016 at 10:53:57AM +0800, Yuanhan Liu wrote:
> On Wed, Dec 14, 2016 at 07:25:30AM +0530, Jerin Jacob wrote:
> > patchset 14-28: Replace the raw readl/writel in the drivers with
> > new rte_read[b/w/l/q], rte_write[b/w/l/q] eal abstraction
>
> Instead of rte_read[b/w/l/q], there is another typical naming style:
> rte_read[8/16/32/64]. Any preferences? If you ask me, I'd prefer the
> later.
No strong opinion here. The rte_read[b/w/l/q] naming style is from Linux
kernel. I will change to rte_read[8/16/32/64] in v2 if there is no
objection.
^ permalink raw reply
* Re: [PATCH 01/22] ethdev: introduce generic flow API
From: Adrien Mazarguil @ 2016-12-14 13:54 UTC (permalink / raw)
To: Kevin Traynor
Cc: dev, Thomas Monjalon, Pablo de Lara, Olivier Matz,
sugesh.chandran
In-Reply-To: <5c53f86b-ead7-b539-6250-40613c7a57db@redhat.com>
Hi Kevin,
On Wed, Dec 14, 2016 at 11:48:04AM +0000, Kevin Traynor wrote:
> hi Adrien, sorry for the delay
>
> <...>
>
> >>>>
> >>>> Is it expected that the application or pmd will provide locking between
> >>>> these functions if required? I think it's going to have to be the app.
> >>>
> >>> Locking is indeed expected to be performed by applications. This API only
> >>> documents places where locking would make sense if necessary and expected
> >>> behavior.
> >>>
> >>> Like all control path APIs, this one assumes a single control thread.
> >>> Applications must take the necessary precautions.
> >>
> >> If you look at OVS now it's quite possible that you have 2 rx queues
> >> serviced by different threads, that would also install the flow rules in
> >> the software flow caches - possibly that could extend to adding hardware
> >> flows. There could also be another thread that is querying for stats. So
> >> anything that can be done to minimise the locking would be helpful -
> >> maybe query() could be atomic and not require any locking?
> >
> > I think we need basic functions with as few constraints as possible on PMDs
> > first, this API being somewhat complex to implement on their side. That
> > covers the common use case where applications have a single control thread
> > or otherwise perform locking on their own.
> >
> > Once the basics are there for most PMDs, we may add new functions, items,
> > properties and actions that provide additional constraints (timing,
> > multi-threading and so on), which remain to be defined according to
> > feedback. It is designed to be extended without causing ABI breakage.
>
> I think Sugesh and I are trying to foresee some of the issues that may
> arise when integrating with something like OVS. OTOH it's
> hard/impossible to say what will be needed exactly in the API right now
> to make it suitable for OVS.
>
> So, I'm ok with the approach you are taking by exposing a basic API
> but I think there should be an expectation that it may not be sufficient
> for a project like OVS to integrate in and may take several
> iterations/extensions - don't go anywhere!
>
> >
> > As for query(), let's see how PMDs handle it first. A race between query()
> > and create() on a given device is almost unavoidable without locking, same
> > for queries that reset counters in a given flow rule. Basic parallel queries
> > should not cause any harm otherwise, although this cannot be guaranteed yet.
>
> You still have a race if there is locking, except it is for the lock,
> but it has the same effect. The downside of my suggestion is that all
> the PMDs would need to guarantee they could gets stats atomically - I'm
> not sure if they can or it's too restrictive.
>
> >
>
> <...>
>
> >>
> >>>
> >>>>> +
> >>>>> +/**
> >>>>> + * Destroy a flow rule on a given port.
> >>>>> + *
> >>>>> + * Failure to destroy a flow rule handle may occur when other flow rules
> >>>>> + * depend on it, and destroying it would result in an inconsistent state.
> >>>>> + *
> >>>>> + * This function is only guaranteed to succeed if handles are destroyed in
> >>>>> + * reverse order of their creation.
> >>>>
> >>>> How can the application find this information out on error?
> >>>
> >>> Without maintaining a list, they cannot. The specified case is the only
> >>> possible guarantee. That does not mean PMDs should not do their best to
> >>> destroy flow rules, only that ordering must remain consistent in case of
> >>> inability to destroy one.
> >>>
> >>> What do you suggest?
> >>
> >> I think if the app cannot remove a specific rule it may want to remove
> >> all rules and deal with flows in software for a time. So once the app
> >> knows it fails that should be enough.
> >
> > OK, then since destruction may return an error already, is it fine?
> > Applications may call rte_flow_flush() (not supposed to fail unless there is
> > a serious issue, abort() in that case) and switch to SW fallback.
>
> yes, it's fine.
>
> >
>
> <...>
>
> >>>>> + * @param[out] error
> >>>>> + * Perform verbose error reporting if not NULL.
> >>>>> + *
> >>>>> + * @return
> >>>>> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> >>>>> + */
> >>>>> +int
> >>>>> +rte_flow_query(uint8_t port_id,
> >>>>> + struct rte_flow *flow,
> >>>>> + enum rte_flow_action_type action,
> >>>>> + void *data,
> >>>>> + struct rte_flow_error *error);
> >>>>> +
> >>>>> +#ifdef __cplusplus
> >>>>> +}
> >>>>> +#endif
> >>>>
> >>>> I don't see a way to dump all the rules for a port out. I think this is
> >>>> neccessary for degbugging. You could have a look through dpif.h in OVS
> >>>> and see how dpif_flow_dump_next() is used, it might be a good reference.
> >>>
> >>> DPDK does not maintain flow rules and, depending on hardware capabilities
> >>> and level of compliance, PMDs do not necessarily do it either, particularly
> >>> since it requires space and application probably have a better method to
> >>> store these pointers for their own needs.
> >>
> >> understood
> >>
> >>>
> >>> What you see here is only a PMD interface. Depending on applications needs,
> >>> generic helper functions built on top of these may be added to manage flow
> >>> rules in the future.
> >>
> >> I'm thinking of the case where something goes wrong and I want to get a
> >> dump of all the flow rules from hardware, not query the rules I think I
> >> have. I don't see a way to do it or something to build a helper on top of?
> >
> > Generic helper functions would exist on top of this API and would likely
> > maintain a list of flow rules themselves. The dump in that case would be
> > entirely implemented in software. I think that recovering flow rules from HW
> > may be complicated in many cases (even without taking storage allocation and
> > rules conversion issues into account), therefore if there is really a need
> > for it, we could perhaps add a dump() function that PMDs are free to
> > implement later.
> >
>
> ok. Maybe there are some more generic stats that can be got from the
> hardware that would help debugging that would suffice, like total flow
> rule hits/misses (i.e. not on a per flow rule basis).
>
> You can get this from the software flow caches and it's widely used for
> debugging. e.g.
>
> pmd thread numa_id 0 core_id 3:
> emc hits:0
> megaflow hits:0
> avg. subtable lookups per hit:0.00
> miss:0
>
Perhaps a rule such as the following could do the trick:
group: 42 (or priority 42)
pattern: void
actions: count / passthru
Assuming useful flow rules are defined with higher priorities (using lower
group ID or priority level) and provide a terminating action, this one would
count all packets that were not caught by them.
That is one example to illustrate how "global" counters can be requested by
applications.
Otherwise you could just make sure all rules contain mark / flag actions, in
which case mbufs would tell directly if they went through them or need
additional SW processing.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH 19/28] net/ena: use eal I/O device memory read/write API
From: Jan Mędala @ 2016-12-14 14:36 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, Ananyev, Konstantin, Thomas Monjalon, Bruce Richardson,
jianbo.liu, viktorin, Santosh Shukla, Jakub Palider,
Alexander Matushevsky
In-Reply-To: <1481680558-4003-20-git-send-email-jerin.jacob@caviumnetworks.com>
Despite the issue with naming convention (either it will be writel or
write32), I'm fine with this change and new API.
Acked-by: Jan Medala <jan@semihalf.com>
Jan
2016-12-14 2:55 GMT+01:00 Jerin Jacob <jerin.jacob@caviumnetworks.com>:
> From: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>
> Replace the raw I/O device memory read/write access with eal
> abstraction for I/O device memory read/write access to fix
> portability issues across different architectures.
>
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: Jan Medala <jan@semihalf.com>
> CC: Jakub Palider <jpa@semihalf.com>
> ---
> drivers/net/ena/base/ena_plat_dpdk.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ena/base/ena_plat_dpdk.h
> b/drivers/net/ena/base/ena_plat_dpdk.h
> index 87c3bf1..4db07c7 100644
> --- a/drivers/net/ena/base/ena_plat_dpdk.h
> +++ b/drivers/net/ena/base/ena_plat_dpdk.h
> @@ -50,6 +50,7 @@
> #include <rte_spinlock.h>
>
> #include <sys/time.h>
> +#include <rte_io.h>
>
> typedef uint64_t u64;
> typedef uint32_t u32;
> @@ -226,12 +227,12 @@ typedef uint64_t dma_addr_t;
>
> static inline void writel(u32 value, volatile void *addr)
> {
> - *(volatile u32 *)addr = value;
> + rte_writel(value, addr);
> }
>
> static inline u32 readl(const volatile void *addr)
> {
> - return *(const volatile u32 *)addr;
> + return rte_readl(addr);
> }
>
> #define ENA_REG_WRITE32(value, reg) writel((value), (reg))
> --
> 2.5.5
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox