[dpdk-dev] [PATCH v2 6/6] mbuf: flatten struct vlan_macip into mbuf struct
Hi Bruce,
Just small typos below.
On 08/28/2014 05:42 PM, Bruce Richardson wrote:
> The vlan_macip structure combined a vlan tag id with l2 and l3 headers
> lengths for tracking offloads. However, this structure was only used as
> a unit by the e1000 and ixgbe drivers, not generally.
>
> This patch removes the structure from the mbuf header and places the
> fields into the mbuf structure directly at the required point, without
> any net effect on the structure layout. This allows us to treat the vlan
> tags and header length fields as separate for future mbuf changes. The
> drivers which were written to use the combined structure still do so,
> using a driver-local definition of it.
>
> Changes in V2:
> * None
>
> [...]
>
> diff --git a/lib/librte_pmd_e1000/em_rxtx.c b/lib/librte_pmd_e1000/em_rxtx.c
> index 074c9b3..4f46bdf 100644
> --- a/lib/librte_pmd_e1000/em_rxtx.c
> +++ b/lib/librte_pmd_e1000/em_rxtx.c
> @@ -144,13 +144,34 @@ enum {
> EM_CTX_NUM = 1, /**< CTX NUM */
> };
>
> +/** Offload features */
> +union em_vlan_macip {
> + uint32_t data;
> + struct {
> + uint16_t l3_len:9; /**< L3 (IP) Header Length. */
> + uint16_t l2_len:7; /**< L2 (MAC) Header Length. */
> + uint16_t vlan_tci;
> + /**< VLAN Tag Control Identifier (CPU order). */
> + } f;
> +};
> +
> +/*
> + * Compare mask for vlan_macip_len.data,
> + * should be in sync with em_vlan_macip.f layout.
> + * */
> +#define TX_VLAN_CMP_MASK0x /**< VLAN length - 16-bits. */
> +#define TX_MAC_LEN_CMP_MASK 0xFE00 /**< MAC length - 7-bits. */
> +#define TX_IP_LEN_CMP_MASK 0x01FF /**< IP length - 9-bits. */
> +/**< MAC+IP length. */
> +#define TX_MACIP_LEN_CMP_MASK (TX_MAC_LEN_CMP_MASK | TX_IP_LEN_CMP_MASK)
> +
I think the doxygen syntax "/**<" is only used for comments that are
after the name. It was already like this before your patch (in
rte_mbuf.h), but maybe it's a good occasion to fix this typo.
There is the same in igb and ixgbe.
> /**
> * Structure to check if new context need be built
> */
> struct em_ctx_info {
> uint16_t flags; /**< ol_flags related to context build. */
> uint32_t cmp_mask;/**< compare mask */
> - union rte_vlan_macip hdrlen; /**< L2 and L3 header lenghts */
> + union em_vlan_macip hdrlen; /**< L2 and L3 header lenghts */
> };
The comment is not aligned with the others.
Acked-by: Olivier Matz
[dpdk-dev] [PATCH v2 6/6] mbuf: flatten struct vlan_macip into mbuf struct
> -Original Message- > From: De Lara Guarch, Pablo > Sent: Friday, September 05, 2014 5:21 PM > To: Richardson, Bruce; dev at dpdk.org > Subject: RE: [dpdk-dev] [PATCH v2 6/6] mbuf: flatten struct vlan_macip into > mbuf struct > > > > > -Original Message- > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Bruce Richardson > > Sent: Thursday, August 28, 2014 4:43 PM > > To: dev at dpdk.org > > Subject: [dpdk-dev] [PATCH v2 6/6] mbuf: flatten struct vlan_macip into > > mbuf struct > > > > The vlan_macip structure combined a vlan tag id with l2 and l3 headers > > lengths for tracking offloads. However, this structure was only used as > > a unit by the e1000 and ixgbe drivers, not generally. > > > > This patch removes the structure from the mbuf header and places the > > fields into the mbuf structure directly at the required point, without > > any net effect on the structure layout. This allows us to treat the vlan > > tags and header length fields as separate for future mbuf changes. The > > drivers which were written to use the combined structure still do so, > > using a driver-local definition of it. > > > > Changes in V2: > > * None > > > > Signed-off-by: Bruce Richardson > > After applying this patch, I see a performance degradation (around 5%) using > testpmd with the default RX path, so this may require a v3 patch. > > Thanks, > Pablo Thanks for Pablo for flagging this. Since no fields are moved in the structure, merely flattened, I suspect any degradation must come from having the l2_len and l3_len bit fields split out. I'll do up a v3 with a union to allow them to be assigned simultaneously and see if it helps things. Fast-path is unaffected in my tests. /Bruce
[dpdk-dev] [PATCH v2 6/6] mbuf: flatten struct vlan_macip into mbuf struct
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Bruce Richardson > Sent: Thursday, August 28, 2014 4:43 PM > To: dev at dpdk.org > Subject: [dpdk-dev] [PATCH v2 6/6] mbuf: flatten struct vlan_macip into > mbuf struct > > The vlan_macip structure combined a vlan tag id with l2 and l3 headers > lengths for tracking offloads. However, this structure was only used as > a unit by the e1000 and ixgbe drivers, not generally. > > This patch removes the structure from the mbuf header and places the > fields into the mbuf structure directly at the required point, without > any net effect on the structure layout. This allows us to treat the vlan > tags and header length fields as separate for future mbuf changes. The > drivers which were written to use the combined structure still do so, > using a driver-local definition of it. > > Changes in V2: > * None > > Signed-off-by: Bruce Richardson After applying this patch, I see a performance degradation (around 5%) using testpmd with the default RX path, so this may require a v3 patch. Thanks, Pablo
[dpdk-dev] [PATCH v2 6/6] mbuf: flatten struct vlan_macip into mbuf struct
The vlan_macip structure combined a vlan tag id with l2 and l3 headers lengths for tracking offloads. However, this structure was only used as a unit by the e1000 and ixgbe drivers, not generally. This patch removes the structure from the mbuf header and places the fields into the mbuf structure directly at the required point, without any net effect on the structure layout. This allows us to treat the vlan tags and header length fields as separate for future mbuf changes. The drivers which were written to use the combined structure still do so, using a driver-local definition of it. Changes in V2: * None Signed-off-by: Bruce Richardson --- app/test-pmd/csumonly.c | 4 +-- app/test-pmd/flowgen.c | 14 +- app/test-pmd/macfwd.c | 6 ++--- app/test-pmd/macswap.c | 6 ++--- app/test-pmd/rxonly.c | 3 +-- app/test-pmd/testpmd.c | 4 ++- app/test-pmd/txonly.c | 6 ++--- app/test/packet_burst_generator.c | 10 examples/ip_fragmentation/main.c| 2 +- examples/ip_pipeline/pipeline_rx.c | 4 +-- examples/ip_pipeline/pipeline_tx.c | 2 +- examples/ip_reassembly/main.c | 8 +++--- examples/ipv4_multicast/main.c | 4 ++- examples/vhost/main.c | 6 ++--- lib/librte_ip_frag/ip_frag_common.h | 3 +-- lib/librte_ip_frag/rte_ipv4_fragmentation.c | 2 +- lib/librte_ip_frag/rte_ipv4_reassembly.c| 6 ++--- lib/librte_ip_frag/rte_ipv6_reassembly.c| 5 ++-- lib/librte_mbuf/rte_mbuf.h | 33 +++- lib/librte_pmd_e1000/em_rxtx.c | 40 ++--- lib/librte_pmd_e1000/igb_rxtx.c | 39 +--- lib/librte_pmd_i40e/i40e_rxtx.c | 14 +- lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 18 ++--- lib/librte_pmd_ixgbe/ixgbe_rxtx.h | 23 - lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c | 6 ++--- 25 files changed, 159 insertions(+), 109 deletions(-) diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index 655b6d8..28b66f5 100644 --- a/app/test-pmd/csumonly.c +++ b/app/test-pmd/csumonly.c @@ -432,8 +432,8 @@ pkt_burst_checksum_forward(struct fwd_stream *fs) } /* Combine the packet header write. VLAN is not consider here */ - mb->vlan_macip.f.l2_len = l2_len; - mb->vlan_macip.f.l3_len = l3_len; + mb->l2_len = l2_len; + mb->l3_len = l3_len; mb->ol_flags = ol_flags; } nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burst, nb_rx); diff --git a/app/test-pmd/flowgen.c b/app/test-pmd/flowgen.c index 17dbf83..b091b6d 100644 --- a/app/test-pmd/flowgen.c +++ b/app/test-pmd/flowgen.c @@ -205,13 +205,13 @@ pkt_burst_flow_gen(struct fwd_stream *fs) udp_hdr->dgram_len = RTE_CPU_TO_BE_16(pkt_size - sizeof(*eth_hdr) - sizeof(*ip_hdr)); - pkt->nb_segs= 1; - pkt->pkt_len= pkt_size; - pkt->ol_flags = ol_flags; - pkt->vlan_macip.f.vlan_tci = vlan_tci; - pkt->vlan_macip.f.l2_len= sizeof(struct ether_hdr); - pkt->vlan_macip.f.l3_len= sizeof(struct ipv4_hdr); - pkts_burst[nb_pkt] = pkt; + pkt->nb_segs= 1; + pkt->pkt_len= pkt_size; + pkt->ol_flags = ol_flags; + pkt->vlan_tci = vlan_tci; + pkt->l2_len = sizeof(struct ether_hdr); + pkt->l3_len = sizeof(struct ipv4_hdr); + pkts_burst[nb_pkt] = pkt; next_flow = (next_flow + 1) % cfg_n_flows; } diff --git a/app/test-pmd/macfwd.c b/app/test-pmd/macfwd.c index 999c8e3..4b905cd 100644 --- a/app/test-pmd/macfwd.c +++ b/app/test-pmd/macfwd.c @@ -116,9 +116,9 @@ pkt_burst_mac_forward(struct fwd_stream *fs) ether_addr_copy([fs->tx_port].eth_addr, _hdr->s_addr); mb->ol_flags = txp->tx_ol_flags; - mb->vlan_macip.f.l2_len = sizeof(struct ether_hdr); - mb->vlan_macip.f.l3_len = sizeof(struct ipv4_hdr); - mb->vlan_macip.f.vlan_tci = txp->tx_vlan_id; + mb->l2_len = sizeof(struct ether_hdr); + mb->l3_len = sizeof(struct ipv4_hdr); + mb->vlan_tci = txp->tx_vlan_id; } nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burst, nb_rx); fs->tx_packets += nb_tx; diff --git
