DPDK patches and discussions
 help / color / mirror / Atom feed
From: Matan Azrad <matan@mellanox.com>
To: Mordechay Haimovsky <motih@mellanox.com>,
	Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3] net/mlx4: support hardware TSO
Date: Thu, 28 Jun 2018 15:19:33 +0000	[thread overview]
Message-ID: <VI1PR0501MB260892EE03EEE25B5046140AD24F0@VI1PR0501MB2608.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <1530190137-17848-1-git-send-email-motih@mellanox.com>

Hi Moti

I started to review it but not finished all :)
Please see some comments\questions,
I will continue the review again in the next version, after addressing the next comments and Adrien comments.

From: Mordechay Haimovsky
> +		 * No TSO SIZE is defined in DPDK, need to figure it out
> +		 * in order to see if we can support it.
> +		 */
> +		mbuf.tso_segsz = size_test;
> +		priv->tso =
> +			((device_attr_ex.tso_caps.max_tso >= mbuf.tso_segsz)
> &&
> +			 (device_attr_ex.tso_caps.supported_qpts &
> +			  (1 << IBV_QPT_RAW_PACKET)));
> +		if (priv->tso)
> +			priv->tso_max_payload_sz =
> +					device_attr_ex.tso_caps.max_tso;

Are all the tso_caps fields exist in old rdma-core versions?

> +		DEBUG("TSO is %ssupported",
> +		      priv->tso ? "" : "not ");
>  		/* Configure the first MAC address by default. */
>  		err = mlx4_get_mac(priv, &mac.addr_bytes);
>  		if (err) {
...
> +mlx4_tx_burst_tso_get_params(struct rte_mbuf *buf,
> +			     struct txq *txq,
> +			     struct tso_info *tinfo)
> +{
> +	struct mlx4_sq *sq = &txq->msq;
> +	const uint8_t tunneled = txq->priv->hw_csum_l2tun &&
> +				 (buf->ol_flags & PKT_TX_TUNNEL_MASK);
> +
> +	tinfo->tso_header_sz = buf->l2_len + buf->l3_len + buf->l4_len;
> +	if (tunneled)
> +		tinfo->tso_header_sz += buf->outer_l2_len + buf-
> >outer_l3_len;

Are those tunnel sizes include outer and inner vlan sizes?

> +	if (unlikely(buf->tso_segsz == 0 || tinfo->tso_header_sz == 0)) {
> +		DEBUG("%p: Invalid TSO parameters", (void *)txq);
> +		return -EINVAL;
> +	}
> +	/* First segment must contain all TSO headers. */
> +	if (unlikely(tinfo->tso_header_sz > MLX4_MAX_TSO_HEADER) ||
> +		     tinfo->tso_header_sz > buf->data_len) {
> +		DEBUG("%p: Invalid TSO header length", (void *)txq);
> +		return -EINVAL;
> +	}
> +	/*
> +	 * Calculate the WQE TSO segment size
> +	 * Note:
> +	 * 1. An LSO segment must be padded such that the subsequent data
> +	 *    segment is 16-byte aligned.
> +	 * 2. The start address of the TSO segment is always 16 Bytes aligned.
> +	 */
...
> +static inline int
> +mlx4_tx_burst_fill_tso_segs(struct rte_mbuf *buf,
> +			    struct txq *txq,
> +			    const struct tso_info *tinfo,
> +			    volatile struct mlx4_wqe_data_seg *dseg,
> +			    struct pv *pv, int *pv_counter)
> +{
> +	uint32_t lkey;
> +	int nb_segs = buf->nb_segs;
> +	int nb_segs_txbb;
> +	struct mlx4_sq *sq = &txq->msq;
> +	struct rte_mbuf *sbuf = buf;
> +	uint16_t sb_of = tinfo->tso_header_sz;
> +	uint16_t data_len;
> +
> +	while (nb_segs > 0) {
> +		/* Wrap dseg if it points at the end of the queue. */
> +		if ((volatile uint8_t *)dseg >= sq->eob)
> +			dseg = (volatile struct mlx4_wqe_data_seg *)
> +					(volatile uint8_t *)dseg - sq->size;

I think we don't need this check in the first time, so maybe move it to the end of the loop is better.

> +		/* how many dseg entries do we have in the current TXBB ? */
> +		nb_segs_txbb =
> +			(MLX4_TXBB_SIZE / sizeof(struct mlx4_wqe_data_seg))
> -
> +			((uintptr_t)dseg & (MLX4_TXBB_SIZE - 1)) /
> +			sizeof(struct mlx4_wqe_data_seg);
> +		switch (nb_segs_txbb) {

I think this switch case is corrupted, what's happen If we have only 1\2\3 segments but we are in the start of txbbb?
You are going to write the first byte of the txbb firstly, no?

> +		case 4:
> +			/* Memory region key for this memory pool. */
> +			lkey = mlx4_tx_mb2mr(txq, sbuf);
> +			if (unlikely(lkey == (uint32_t)-1))
> +				goto lkey_err;
> +			dseg->addr =
> +			    rte_cpu_to_be_64(rte_pktmbuf_mtod_offset(sbuf,
> +								     uintptr_t,
> +								     sb_of));
> +			dseg->lkey = lkey;
> +			/*
> +			 * This data segment starts at the beginning of a new
> +			 * TXBB, so we need to postpone its byte_count writing
> +			 * for later.
> +			 */
> +			pv[*pv_counter].dseg = dseg;
> +			/*
> +			 * Zero length segment is treated as inline segment
> +			 * with zero data.
> +			 */
> +			data_len = sbuf->data_len - sb_of;
> +			pv[(*pv_counter)++].val =
> +				rte_cpu_to_be_32(data_len ?
> +						 data_len :
> +						 0x80000000);
> +			sb_of = 0;
> +			sbuf = sbuf->next;
> +			dseg++;
> +			if (--nb_segs == 0)
> +				break;
> +			/* fallthrough */
> +		case 3:
> +			lkey = mlx4_tx_mb2mr(txq, sbuf);
> +			if (unlikely(lkey == (uint32_t)-1))
> +				goto lkey_err;
> +			data_len = sbuf->data_len - sb_of;
> +			mlx4_fill_tx_data_seg(dseg,
> +					lkey,
> +					rte_pktmbuf_mtod_offset(sbuf,
> +								uintptr_t,
> +								sb_of),
> +					rte_cpu_to_be_32(data_len ?
> +							 data_len :
> +							 0x80000000));
> +			sb_of = 0;
> +			sbuf = sbuf->next;
> +			dseg++;
> +			if (--nb_segs == 0)
> +				break;
> +			/* fallthrough */
> +		case 2:
> +			lkey = mlx4_tx_mb2mr(txq, sbuf);
> +			if (unlikely(lkey == (uint32_t)-1))
> +				goto lkey_err;
> +			data_len = sbuf->data_len - sb_of;
> +			mlx4_fill_tx_data_seg(dseg,
> +					lkey,
> +					rte_pktmbuf_mtod_offset(sbuf,
> +								uintptr_t,
> +								sb_of),
> +					rte_cpu_to_be_32(data_len ?
> +							 data_len :
> +							 0x80000000));
> +			sb_of = 0;
> +			sbuf = sbuf->next;
> +			dseg++;
> +			if (--nb_segs == 0)
> +				break;
> +			/* fallthrough */
> +		case 1:
> +			lkey = mlx4_tx_mb2mr(txq, sbuf);
> +			if (unlikely(lkey == (uint32_t)-1))
> +				goto lkey_err;
> +			data_len = sbuf->data_len - sb_of;
> +			mlx4_fill_tx_data_seg(dseg,
> +					lkey,
> +					rte_pktmbuf_mtod_offset(sbuf,
> +								uintptr_t,
> +								sb_of),
> +					rte_cpu_to_be_32(data_len ?
> +							 data_len :
> +							 0x80000000));
> +			sb_of = 0;
> +			sbuf = sbuf->next;
> +			dseg++;
> +			--nb_segs;
> +			break;
> +		default:
> +			/* Should never happen */
> +			ERROR("%p: invalid number of txbb data segments
> %d",
> +			      (void *)txq, nb_segs_txbb);
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +lkey_err:
> +	DEBUG("%p: unable to get MP <-> MR association",
> +	      (void *)txq);
> +	return -EFAULT;
> +}

Matan

  parent reply	other threads:[~2018-06-28 15:19 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <0~1530184583-30166-1-git-send-email-motih@mellanox.com>
2018-06-28 11:57 ` [dpdk-dev] [PATCH v2] " Moti Haimovsky
2018-06-28 12:48   ` [dpdk-dev] [PATCH v3] " Moti Haimovsky
2018-06-28 14:15     ` Adrien Mazarguil
2018-06-28 15:19     ` Matan Azrad [this message]
2018-07-04 14:53     ` [dpdk-dev] [PATCH v4] " Moti Haimovsky
2018-07-05 12:30       ` Matan Azrad
2018-07-09 10:43       ` [dpdk-dev] [PATCH v5] " Moti Haimovsky
2018-07-09 13:07         ` Matan Azrad
2018-07-09 16:22           ` Mordechay Haimovsky
2018-07-09 18:18             ` Matan Azrad
2018-07-09 16:33         ` [dpdk-dev] [PATCH v6] " Moti Haimovsky
2018-07-10 10:45           ` [dpdk-dev] [PATCH v7] " Moti Haimovsky
2018-07-10 11:02             ` Matan Azrad
2018-07-10 12:03               ` Shahaf Shuler

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=VI1PR0501MB260892EE03EEE25B5046140AD24F0@VI1PR0501MB2608.eurprd05.prod.outlook.com \
    --to=matan@mellanox.com \
    --cc=adrien.mazarguil@6wind.com \
    --cc=dev@dpdk.org \
    --cc=motih@mellanox.com \
    /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).