From: "Liang, Cunming" <cunming.liang@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
Zoltan Kiss <zoltan.kiss@linaro.org>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v1] ixgbe: remove vector pmd burst size restriction
Date: Mon, 3 Aug 2015 07:41:05 +0000 [thread overview]
Message-ID: <D0158A423229094DA7ABF71CF2FA0DA3119B317E@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB97725836A6B79C@irsmsx105.ger.corp.intel.com>
Hi,
[...]
> > > uint16_t
> > > ixgbe_recv_scattered_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
> > > uint16_t nb_pkts)
> > > {
> > > struct ixgbe_rx_queue *rxq = rx_queue;
> > > - uint8_t split_flags[RTE_IXGBE_VPMD_RX_BURST] = {0};
> > > + uint8_t split_flags[nb_pkts];
> > > +
> > > + memset(split_flags, 0, nb_pkts);
> > >
> > > /* get some new buffers */
> > > uint16_t nb_bufs = _recv_raw_pkts_vec(rxq, rx_pkts, nb_pkts,
> >
> > After this _recv_raw_pkts_vec it checks 32 bytes in split_flags (4x8
> > bytes), that can overrun or miss some flags.
> > Btw. Bruce just fixed that part in "ixgbe: fix check for split packets"
>
> Ah yes, missed that when reviewing, that code would be broken if nb_bufs > 32:
>
> const uint64_t *split_fl64 = (uint64_t *)split_flags;
> if (rxq->pkt_first_seg == NULL &&
> split_fl64[0] == 0 && split_fl64[1] == 0 &&
> split_fl64[2] == 0 && split_fl64[3] == 0)
> return nb_bufs;
>
> right?
We can either rollback and only allow 'nb_pkts<=32', or do some broken fix as below diff.
By the result of performance test (4*10GE 64B burst_size(32) iofwd by scattered_pkts_vec), there's no drop.
But I'm not sure it is important or not to allow burst size larger than 32. Your comments will be important.
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index e94c68b..8f34236 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -537,26 +537,35 @@ uint16_t
ixgbe_recv_scattered_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
uint16_t nb_pkts)
{
+#define NB_SPLIT_ELEM (8)
struct ixgbe_rx_queue *rxq = rx_queue;
uint8_t split_flags[nb_pkts];
+ uint32_t i, nb_scan;
+ uint16_t nb_bufs;
+ uint64_t *split_fl64 = (uint64_t *)split_flags;
memset(split_flags, 0, nb_pkts);
/* get some new buffers */
- uint16_t nb_bufs = _recv_raw_pkts_vec(rxq, rx_pkts, nb_pkts,
- split_flags);
+ nb_bufs = _recv_raw_pkts_vec(rxq, rx_pkts, nb_pkts,
+ split_flags);
if (nb_bufs == 0)
return 0;
/* happy day case, full burst + no packets to be joined */
- const uint64_t *split_fl64 = (uint64_t *)split_flags;
- if (rxq->pkt_first_seg == NULL &&
- split_fl64[0] == 0 && split_fl64[1] == 0 &&
- split_fl64[2] == 0 && split_fl64[3] == 0)
+ nb_scan = RTE_ALIGN(nb_bufs, NB_SPLIT_ELEM);
+ if (rxq->pkt_first_seg == NULL) {
+ for (i = 0; i < nb_scan;
+ i += NB_SPLIT_ELEM, split_fl64++) {
+ if (*split_fl64 != 0)
+ goto reassemble;
+ }
return nb_bufs;
+ }
+reassemble:
/* reassemble any packets that need reassembly*/
- unsigned i = 0;
+ i = 0;
if (rxq->pkt_first_seg == NULL) {
/* find the first split flag, and only reassemble then*/
while (i < nb_bufs && !split_flags[i])
/Steve
>
> Another thing, that I just thought about:
> Right now we invoke ixgbe_rxq_rearm() only at the start of
> _recv_raw_pkts_vec().
> Before it was ok, as _recv_raw_pkts_vec() would never try to read more then 32
> RXDs.
> But what would happen if nb_pkts > rxq->nb_desc and rxq->rxrearm_nb == 0?
> I suppose, _recv_raw_pkts_vec() can wrpa around RXD ring and 'receive' same
> packet twice?
> So we probably better still limit nb_pkts <= 32 at _recv_raw_pkts_vec().
The _recv_raw_pkts_vec() won't wrap around RXD ring. When it reaches the last one, the DD bit of padding desc. always 0.
So in the case nb_pkts > rxq->nb_desc, the '_recv_raw_pkts_vec()' can only get no more than 'rxq->nb_desc' packets.
>
> Konstantin
>
> >
next prev parent reply other threads:[~2015-08-03 7:41 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-31 8:17 Cunming Liang
2015-07-31 9:21 ` Ananyev, Konstantin
2015-07-31 10:03 ` Zoltan Kiss
2015-07-31 10:21 ` Ananyev, Konstantin
2015-07-31 11:57 ` Zoltan Kiss
2015-07-31 14:49 ` Zoltan Kiss
2015-08-03 7:41 ` Liang, Cunming [this message]
2015-08-03 9:59 ` Liang, Cunming
2015-08-03 2:40 ` Liang, Cunming
2015-08-04 7:32 ` [dpdk-dev] [PATCH v2] " Cunming Liang
2015-08-04 9:02 ` Zoltan Kiss
2015-08-04 11:15 ` Liang, Cunming
2015-08-04 11:47 ` [dpdk-dev] [PATCH v3] " Cunming Liang
2015-08-04 16:26 ` Zoltan Kiss
2015-08-05 6:28 ` Liang, Cunming
2015-08-05 15:59 ` Zoltan Kiss
2015-08-05 9:31 ` Ananyev, Konstantin
2015-09-09 13:16 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D0158A423229094DA7ABF71CF2FA0DA3119B317E@shsmsx102.ccr.corp.intel.com \
--to=cunming.liang@intel.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@intel.com \
--cc=zoltan.kiss@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).