[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On 03/04/15 20:54, Stephen Hemminger wrote: > On Wed, 04 Mar 2015 09:57:24 +0200 > Vlad Zolotarov wrote: > >> >> On 03/04/15 02:33, Stephen Hemminger wrote: >>> On Tue, 3 Mar 2015 21:48:43 +0200 >>> Vlad Zolotarov wrote: >>> + next_desc: + /* + * The code in this whole file uses the volatile pointer to + * ensure the read ordering of the status and the rest of the + * descriptor fields (on the compiler level only!!!). This is so + * UGLY - why not to just use the compiler barrier instead? DPDK + * even has the rte_compiler_barrier() for that. + * + * But most importantly this is just wrong because this doesn't + * ensure memory ordering in a general case at all. For + * instance, DPDK is supposed to work on Power CPUs where + * compiler barrier may just not be enough! + * + * I tried to write only this function properly to have a + * starting point (as a part of an LRO/RSC series) but the + * compiler cursed at me when I tried to cast away the + * "volatile" from rx_ring (yes, it's volatile too!!!). So, I'm + * keeping it the way it is for now. + * + * The code in this file is broken in so many other places and + * will just not work on a big endian CPU anyway therefore the + * lines below will have to be revisited together with the rest + * of the ixgbe PMD. + * + * TODO: + *- Get rid of "volatile" crap and let the compiler do its + * job. + *- Use the proper memory barrier (rte_rmb()) to ensure the + * memory ordering below. >>> This comment screams "this is broken". >>> Why not get proper architecture independent barriers in DPDK first. >> This series is orthogonal to the issue above. I just couldn't stand to >> mention this ugliness when I noticed it on the way. >> Note that although this is obviously not the right way to write this >> kind of code it is still not a bug and most likely the performance >> implications are minimal here. >> The only overhead is that there may be read "too much" data from the >> descriptor that we may not actually need. The descriptor is 16 bytes so >> this doesn't seem to be a critical issue. >> >> So, fixing the above issue may wait, especially since the same s..t may >> be found in other Intel PMDs (see i40e for example). Fixing this issue >> should be a matter of a massive cleanup series that cover all the >> relevant PMDs. Of course we may start with ixgbe but even in this single >> PMD there are at least 3 non-LRO related functions that have to be >> fixed, so IMHO even fixing ONLY ixgbe should be a matter of a separate >> series. > In userspace-rcu and kernel there is a simple macro that would make this > kind of code more sane. > > What about adding: > > #define rte_access_once(x) (*(volatile typeof(x) *)&(x)) > > Then doing > rxdp = rte_access_once(rx_ring + idx); This workaround doesn't address the described above issue - it just hides it inside a macro, which is even uglier. The main reason I haven't fixed this issue in (at least) a function I've added is that the hw->rx_ring (HW ring) is defined as volatile and this fact is used all over the file in different places and all such places have to be fixed if I drop the "volatile" qualifier which should be the first thing to do. > > > >
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On Wed, 04 Mar 2015 09:57:24 +0200 Vlad Zolotarov wrote: > > > On 03/04/15 02:33, Stephen Hemminger wrote: > > On Tue, 3 Mar 2015 21:48:43 +0200 > > Vlad Zolotarov wrote: > > > >> + next_desc: > >> + /* > >> + * The code in this whole file uses the volatile pointer to > >> + * ensure the read ordering of the status and the rest of the > >> + * descriptor fields (on the compiler level only!!!). This is so > >> + * UGLY - why not to just use the compiler barrier instead? DPDK > >> + * even has the rte_compiler_barrier() for that. > >> + * > >> + * But most importantly this is just wrong because this doesn't > >> + * ensure memory ordering in a general case at all. For > >> + * instance, DPDK is supposed to work on Power CPUs where > >> + * compiler barrier may just not be enough! > >> + * > >> + * I tried to write only this function properly to have a > >> + * starting point (as a part of an LRO/RSC series) but the > >> + * compiler cursed at me when I tried to cast away the > >> + * "volatile" from rx_ring (yes, it's volatile too!!!). So, I'm > >> + * keeping it the way it is for now. > >> + * > >> + * The code in this file is broken in so many other places and > >> + * will just not work on a big endian CPU anyway therefore the > >> + * lines below will have to be revisited together with the rest > >> + * of the ixgbe PMD. > >> + * > >> + * TODO: > >> + *- Get rid of "volatile" crap and let the compiler do its > >> + * job. > >> + *- Use the proper memory barrier (rte_rmb()) to ensure the > >> + * memory ordering below. > > This comment screams "this is broken". > > Why not get proper architecture independent barriers in DPDK first. > > This series is orthogonal to the issue above. I just couldn't stand to > mention this ugliness when I noticed it on the way. > Note that although this is obviously not the right way to write this > kind of code it is still not a bug and most likely the performance > implications are minimal here. > The only overhead is that there may be read "too much" data from the > descriptor that we may not actually need. The descriptor is 16 bytes so > this doesn't seem to be a critical issue. > > So, fixing the above issue may wait, especially since the same s..t may > be found in other Intel PMDs (see i40e for example). Fixing this issue > should be a matter of a massive cleanup series that cover all the > relevant PMDs. Of course we may start with ixgbe but even in this single > PMD there are at least 3 non-LRO related functions that have to be > fixed, so IMHO even fixing ONLY ixgbe should be a matter of a separate > series. In userspace-rcu and kernel there is a simple macro that would make this kind of code more sane. What about adding: #define rte_access_once(x) (*(volatile typeof(x) *)&(x)) Then doing rxdp = rte_access_once(rx_ring + idx);
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On Wed, 04 Mar 2015 09:59:38 +0200 Vlad Zolotarov wrote: > > Checkpatch warnings (edited to remove ones that should be ok) > > I was unaware that checkpatch rules apply here - at least looking at the > current code it looks like it... ;) > But I'm all for it! I'll fix all the issues and respin Checkpatch doesn't generally have to apply. But is good and useful tool to find obvious style gaffs. Just don't let it rule your world
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On 03/04/2015 02:33 AM, Stephen Hemminger wrote: > On Tue, 3 Mar 2015 21:48:43 +0200 > Vlad Zolotarov wrote: > >> + * TODO: >> + *- Get rid of "volatile" crap and let the compiler do its >> + * job. >> + *- Use the proper memory barrier (rte_rmb()) to ensure the >> + * memory ordering below. > This comment screams "this is broken". > Why not get proper architecture independent barriers in DPDK first. C11 has arch independent memory barriers, so this can be as simple as -std=gnu11 (default in gcc 5, anyway). Not only do we get the barriers for free, but they are also properly integrated with the compiler, so for example a release barrier won't stop the compiler from hoisting a later accesses to before the store, or cause spurious reloads, due to the memory clobber.
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On 03/04/15 02:36, Stephen Hemminger wrote:
> On Tue, 3 Mar 2015 21:48:43 +0200
> Vlad Zolotarov wrote:
>
>> - Only x540 and 82599 devices support LRO.
>> - Add the appropriate HW configuration.
>> - Add RSC aware rx_pkt_burst() handlers:
>> - Implemented bulk allocation and non-bulk allocation versions.
>> - Add LRO-specific fields to rte_eth_rxmode, to rte_eth_dev_data
>> and to igb_rx_queue.
>> - Use the appropriate handler when LRO is requested.
>>
>> Signed-off-by: Vlad Zolotarov
> Checkpatch warnings (edited to remove ones that should be ok)
I was unaware that checkpatch rules apply here - at least looking at the
current code it looks like it... ;)
But I'm all for it! I'll fix all the issues and respin.
Thanks.
>
>
> WARNING: 'recieved' may be misspelled - perhaps 'received'?
> #196: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1426:
> + * @rx_pkts table of recieved packets
>
> WARNING: Missing a blank line after declarations
> #223: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1453:
> + struct igb_rx_queue *rxq = rx_queue;
> + volatile union ixgbe_adv_rx_desc *rx_ring = rxq->rx_ring;
>
>
>
> WARNING: labels should not be indented
> #246: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1476:
> + next_desc:
>
> WARNING: quoted string split across lines
> #285: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1515:
> + PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_id=%u "
> + "staterr=0x%x data_len=%u",
>
> WARNING: quoted string split across lines
> #293: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1523:
> + PMD_RX_LOG(DEBUG, "RX mbuf alloc failed "
> + "port_id=%u queue_id=%u",
>
> WARNING: Missing a blank line after declarations
> #302: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1532:
> + uint16_t next_rdt = rxq->rx_free_trigger;
> + if (!ixgbe_rx_alloc_bufs(rxq, false)) {
>
> WARNING: quoted string split across lines
> #309: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1539:
> + PMD_RX_LOG(DEBUG, "RX bulk alloc failed "
> + "port_id=%u queue_id=%u",
>
> ERROR: code indent should use tabs where possible
> #350: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1580:
> +rxm->data_off = RTE_PKTMBUF_HEADROOM;$
>
> WARNING: please, no spaces at the start of a line
> #350: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1580:
> +rxm->data_off = RTE_PKTMBUF_HEADROOM;$
>
> WARNING: quoted string split across lines
> #452: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1682:
> + PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_tail=%u "
> +"nb_hold=%u nb_rx=%u",
>
> WARNING: quoted string split across lines
> #536: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2580:
> + PMD_INIT_LOG(DEBUG, "sw_ring=%p sw_rsc_ring=%p hw_ring=%p "
> + "dma_addr=0x%"PRIx64,
>
> WARNING: Possible switch case/default not preceeded by break or fallthrough
> comment
> #617: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3926:
> + default:
>
> WARNING: quoted string split across lines
> #648: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3966:
> + PMD_INIT_LOG(CRIT, "LRO is requested on HW that doesn't "
> +"support it");
>
> WARNING: quoted string split across lines
> #693: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4022:
> + PMD_INIT_LOG(CRIT, "LRO can't be enabled when HW CRC "
> + "is disabled");
>
> WARNING: quoted string split across lines
> #711: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4084:
> + PMD_INIT_LOG(INFO, "split_hdr_size less than "
> +"128 bytes (%d)!",
>
> WARNING: quoted string split across lines
> #835: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4267:
> + PMD_INIT_LOG(INFO, "LRO is requested. Using a bulk "
> +"allocation version");
>
> WARNING: quoted string split across lines
> #840: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4272:
> + PMD_INIT_LOG(INFO, "LRO is requested. Using a single "
> +"allocation version");
>
>
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On 03/04/15 02:34, Stephen Hemminger wrote:
> On Tue, 3 Mar 2015 21:48:43 +0200
> Vlad Zolotarov wrote:
>
>> +
>> +if (!bulk_alloc) {
>> +__le64 dma =
>> + rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(nmb));
>> +/*
>> + * Update RX descriptor with the physical address of the
>> + * new data buffer of the new allocated mbuf.
>> + */
>> +rxe->mbuf = nmb;
>> +
>> +rxm->data_off = RTE_PKTMBUF_HEADROOM;
> Incorrect indentation.
Oops... ;)
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On 03/04/15 02:33, Stephen Hemminger wrote: > On Tue, 3 Mar 2015 21:48:43 +0200 > Vlad Zolotarov wrote: > >> +next_desc: >> +/* >> + * The code in this whole file uses the volatile pointer to >> + * ensure the read ordering of the status and the rest of the >> + * descriptor fields (on the compiler level only!!!). This is so >> + * UGLY - why not to just use the compiler barrier instead? DPDK >> + * even has the rte_compiler_barrier() for that. >> + * >> + * But most importantly this is just wrong because this doesn't >> + * ensure memory ordering in a general case at all. For >> + * instance, DPDK is supposed to work on Power CPUs where >> + * compiler barrier may just not be enough! >> + * >> + * I tried to write only this function properly to have a >> + * starting point (as a part of an LRO/RSC series) but the >> + * compiler cursed at me when I tried to cast away the >> + * "volatile" from rx_ring (yes, it's volatile too!!!). So, I'm >> + * keeping it the way it is for now. >> + * >> + * The code in this file is broken in so many other places and >> + * will just not work on a big endian CPU anyway therefore the >> + * lines below will have to be revisited together with the rest >> + * of the ixgbe PMD. >> + * >> + * TODO: >> + *- Get rid of "volatile" crap and let the compiler do its >> + * job. >> + *- Use the proper memory barrier (rte_rmb()) to ensure the >> + * memory ordering below. > This comment screams "this is broken". > Why not get proper architecture independent barriers in DPDK first. This series is orthogonal to the issue above. I just couldn't stand to mention this ugliness when I noticed it on the way. Note that although this is obviously not the right way to write this kind of code it is still not a bug and most likely the performance implications are minimal here. The only overhead is that there may be read "too much" data from the descriptor that we may not actually need. The descriptor is 16 bytes so this doesn't seem to be a critical issue. So, fixing the above issue may wait, especially since the same s..t may be found in other Intel PMDs (see i40e for example). Fixing this issue should be a matter of a massive cleanup series that cover all the relevant PMDs. Of course we may start with ixgbe but even in this single PMD there are at least 3 non-LRO related functions that have to be fixed, so IMHO even fixing ONLY ixgbe should be a matter of a separate series.
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On 03/04/15 02:33, Stephen Hemminger wrote: > On Tue, 3 Mar 2015 21:48:43 +0200 > Vlad Zolotarov wrote: > >> +lro_bulk_alloc: 1, /**< RX LRO with bulk alloc is ON(1) / >> OFF(0) */ > This is an internal decision and should not be exposed in the API. > We need less knobs not more. I get your point. I'll move this to ixgbe-specific dev data.
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
- Only x540 and 82599 devices support LRO.
- Add the appropriate HW configuration.
- Add RSC aware rx_pkt_burst() handlers:
- Implemented bulk allocation and non-bulk allocation versions.
- Add LRO-specific fields to rte_eth_rxmode, to rte_eth_dev_data
and to igb_rx_queue.
- Use the appropriate handler when LRO is requested.
Signed-off-by: Vlad Zolotarov
---
lib/librte_ether/rte_ethdev.h | 7 +-
lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 17 ++
lib/librte_pmd_ixgbe/ixgbe_ethdev.h | 5 +
lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 563 +++-
lib/librte_pmd_ixgbe/ixgbe_rxtx.h | 6 +
5 files changed, 591 insertions(+), 7 deletions(-)
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 8db3127..f5eff81 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -320,14 +320,15 @@ struct rte_eth_rxmode {
enum rte_eth_rx_mq_mode mq_mode;
uint32_t max_rx_pkt_len; /**< Only used if jumbo_frame enabled. */
uint16_t split_hdr_size; /**< hdr buf size (header_split enabled).*/
- uint8_t header_split : 1, /**< Header Split enable. */
+ uint16_t header_split : 1, /**< Header Split enable. */
hw_ip_checksum : 1, /**< IP/UDP/TCP checksum offload enable.
*/
hw_vlan_filter : 1, /**< VLAN filter enable. */
hw_vlan_strip: 1, /**< VLAN strip enable. */
hw_vlan_extend : 1, /**< Extended VLAN enable. */
jumbo_frame : 1, /**< Jumbo Frame Receipt enable. */
hw_strip_crc : 1, /**< Enable CRC stripping by hardware. */
- enable_scatter : 1; /**< Enable scatter packets rx handler */
+ enable_scatter : 1, /**< Enable scatter packets rx handler */
+ enable_lro : 1; /**< Enable LRO */
};
/**
@@ -1515,6 +1516,8 @@ struct rte_eth_dev_data {
uint8_t port_id; /**< Device [external] port identifier. */
uint8_t promiscuous : 1, /**< RX promiscuous mode ON(1) / OFF(0). */
scattered_rx : 1, /**< RX of scattered packets is ON(1) /
OFF(0) */
+ lro : 1, /**< RX LRO is ON(1) / OFF(0) */
+ lro_bulk_alloc: 1, /**< RX LRO with bulk alloc is ON(1) /
OFF(0) */
all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
dev_started : 1; /**< Device state: STARTED(1) / STOPPED(0).
*/
};
diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
index 9bdc046..a5a4cb8 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
@@ -762,6 +762,14 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct
eth_driver *eth_drv,
if (eth_dev->data->scattered_rx)
eth_dev->rx_pkt_burst = ixgbe_recv_scattered_pkts;
+
+ if (eth_dev->data->lro) {
+ if (eth_dev->data->lro_bulk_alloc)
+ eth_dev->rx_pkt_burst =
ixgbe_recv_pkts_lro_bulk_alloc;
+ else
+ eth_dev->rx_pkt_burst = ixgbe_recv_pkts_lro;
+ }
+
return 0;
}
pci_dev = eth_dev->pci_dev;
@@ -1641,6 +1649,8 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
/* Clear stored conf */
dev->data->scattered_rx = 0;
+ dev->data->lro = 0;
+ dev->data->lro_bulk_alloc = 0;
/* Clear recorded link status */
memset(&link, 0, sizeof(link));
@@ -2009,6 +2019,13 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct
rte_eth_dev_info *dev_info)
DEV_RX_OFFLOAD_IPV4_CKSUM |
DEV_RX_OFFLOAD_UDP_CKSUM |
DEV_RX_OFFLOAD_TCP_CKSUM;
+
+#ifdef RTE_ETHDEV_LRO_SUPPORT
+ if (hw->mac.type == ixgbe_mac_82599EB ||
+ hw->mac.type == ixgbe_mac_X540)
+ dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_TCP_LRO;
+#endif
+
dev_info->tx_offload_capa =
DEV_TX_OFFLOAD_VLAN_INSERT |
DEV_TX_OFFLOAD_IPV4_CKSUM |
diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
index a549f5c..e206584 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
+++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.h
@@ -349,6 +349,11 @@ uint16_t ixgbe_recv_pkts_bulk_alloc(void *rx_queue, struct
rte_mbuf **rx_pkts,
uint16_t ixgbe_recv_scattered_pkts(void *rx_queue,
struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
+uint16_t ixgbe_recv_pkts_lro(void *rx_queue,
+ struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
+uint16_t ixgbe_recv_pkts_lro_bulk_alloc(void *rx_queue,
+ struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
+
uint16_t ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
b/li
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On Tue, 3 Mar 2015 21:48:43 +0200
Vlad Zolotarov wrote:
> - Only x540 and 82599 devices support LRO.
> - Add the appropriate HW configuration.
> - Add RSC aware rx_pkt_burst() handlers:
>- Implemented bulk allocation and non-bulk allocation versions.
>- Add LRO-specific fields to rte_eth_rxmode, to rte_eth_dev_data
> and to igb_rx_queue.
>- Use the appropriate handler when LRO is requested.
>
> Signed-off-by: Vlad Zolotarov
Checkpatch warnings (edited to remove ones that should be ok)
WARNING: 'recieved' may be misspelled - perhaps 'received'?
#196: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1426:
+ * @rx_pkts table of recieved packets
WARNING: Missing a blank line after declarations
#223: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1453:
+ struct igb_rx_queue *rxq = rx_queue;
+ volatile union ixgbe_adv_rx_desc *rx_ring = rxq->rx_ring;
WARNING: labels should not be indented
#246: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1476:
+ next_desc:
WARNING: quoted string split across lines
#285: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1515:
+ PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_id=%u "
+ "staterr=0x%x data_len=%u",
WARNING: quoted string split across lines
#293: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1523:
+ PMD_RX_LOG(DEBUG, "RX mbuf alloc failed "
+ "port_id=%u queue_id=%u",
WARNING: Missing a blank line after declarations
#302: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1532:
+ uint16_t next_rdt = rxq->rx_free_trigger;
+ if (!ixgbe_rx_alloc_bufs(rxq, false)) {
WARNING: quoted string split across lines
#309: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1539:
+ PMD_RX_LOG(DEBUG, "RX bulk alloc failed "
+ "port_id=%u queue_id=%u",
ERROR: code indent should use tabs where possible
#350: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1580:
+rxm->data_off = RTE_PKTMBUF_HEADROOM;$
WARNING: please, no spaces at the start of a line
#350: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1580:
+rxm->data_off = RTE_PKTMBUF_HEADROOM;$
WARNING: quoted string split across lines
#452: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1682:
+ PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_tail=%u "
+ "nb_hold=%u nb_rx=%u",
WARNING: quoted string split across lines
#536: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2580:
+ PMD_INIT_LOG(DEBUG, "sw_ring=%p sw_rsc_ring=%p hw_ring=%p "
+ "dma_addr=0x%"PRIx64,
WARNING: Possible switch case/default not preceeded by break or fallthrough
comment
#617: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3926:
+ default:
WARNING: quoted string split across lines
#648: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3966:
+ PMD_INIT_LOG(CRIT, "LRO is requested on HW that doesn't "
+ "support it");
WARNING: quoted string split across lines
#693: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4022:
+ PMD_INIT_LOG(CRIT, "LRO can't be enabled when HW CRC "
+ "is disabled");
WARNING: quoted string split across lines
#711: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4084:
+ PMD_INIT_LOG(INFO, "split_hdr_size less than "
+ "128 bytes (%d)!",
WARNING: quoted string split across lines
#835: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4267:
+ PMD_INIT_LOG(INFO, "LRO is requested. Using a bulk "
+ "allocation version");
WARNING: quoted string split across lines
#840: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4272:
+ PMD_INIT_LOG(INFO, "LRO is requested. Using a single "
+ "allocation version");
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On Tue, 3 Mar 2015 21:48:43 +0200
Vlad Zolotarov wrote:
> +
> + if (!bulk_alloc) {
> + __le64 dma =
> + rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(nmb));
> + /*
> + * Update RX descriptor with the physical address of the
> + * new data buffer of the new allocated mbuf.
> + */
> + rxe->mbuf = nmb;
> +
> +rxm->data_off = RTE_PKTMBUF_HEADROOM;
Incorrect indentation.
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On Tue, 3 Mar 2015 21:48:43 +0200 Vlad Zolotarov wrote: > + next_desc: > + /* > + * The code in this whole file uses the volatile pointer to > + * ensure the read ordering of the status and the rest of the > + * descriptor fields (on the compiler level only!!!). This is so > + * UGLY - why not to just use the compiler barrier instead? DPDK > + * even has the rte_compiler_barrier() for that. > + * > + * But most importantly this is just wrong because this doesn't > + * ensure memory ordering in a general case at all. For > + * instance, DPDK is supposed to work on Power CPUs where > + * compiler barrier may just not be enough! > + * > + * I tried to write only this function properly to have a > + * starting point (as a part of an LRO/RSC series) but the > + * compiler cursed at me when I tried to cast away the > + * "volatile" from rx_ring (yes, it's volatile too!!!). So, I'm > + * keeping it the way it is for now. > + * > + * The code in this file is broken in so many other places and > + * will just not work on a big endian CPU anyway therefore the > + * lines below will have to be revisited together with the rest > + * of the ixgbe PMD. > + * > + * TODO: > + *- Get rid of "volatile" crap and let the compiler do its > + * job. > + *- Use the proper memory barrier (rte_rmb()) to ensure the > + * memory ordering below. This comment screams "this is broken". Why not get proper architecture independent barriers in DPDK first.
[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
On Tue, 3 Mar 2015 21:48:43 +0200 Vlad Zolotarov wrote: > + lro_bulk_alloc: 1, /**< RX LRO with bulk alloc is ON(1) / > OFF(0) */ This is an internal decision and should not be exposed in the API. We need less knobs not more.
