From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v3 7/7] net/mlx4: remove empty Tx segment support Date: Mon, 30 Oct 2017 15:24:03 +0100 Message-ID: <20171030142403.GD26782@6wind.com> References: <1508768520-4810-1-git-send-email-ophirmu@mellanox.com> <1509358049-18854-1-git-send-email-matan@mellanox.com> <1509358049-18854-8-git-send-email-matan@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, Ophir Munk To: Matan Azrad Return-path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id 5B2A81B322 for ; Mon, 30 Oct 2017 15:24:15 +0100 (CET) Received: by mail-wm0-f67.google.com with SMTP id t139so16831303wmt.1 for ; Mon, 30 Oct 2017 07:24:15 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1509358049-18854-8-git-send-email-matan@mellanox.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Oct 30, 2017 at 10:07:29AM +0000, Matan Azrad wrote: > Move empty segment case processing to debug mode. > > Signed-off-by: Matan Azrad Whoa, I think there's a misunderstanding here. Nothing prevents applications from attempting to send zero-length segments, and the PMD must survive this somehow. I think this commit should be dropped, more below. > --- > drivers/net/mlx4/mlx4_rxtx.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c > index 482c399..c005a41 100644 > --- a/drivers/net/mlx4/mlx4_rxtx.c > +++ b/drivers/net/mlx4/mlx4_rxtx.c > @@ -305,15 +305,18 @@ static int handle_multi_segs(struct rte_mbuf *buf, > return -1; > } > #endif /* NDEBUG */ > - if (likely(sbuf->data_len)) { > - byte_count = rte_cpu_to_be_32(sbuf->data_len); > - } else { > + byte_count = rte_cpu_to_be_32(sbuf->data_len); > +#ifndef NDEBUG > + if (unlikely(!sbuf->data_len)) { > + DEBUG("%p: Empty segment is not allowed", > + (void *)txq); > /* > * Zero length segment is treated as inline segment > * with zero data. > */ > byte_count = RTE_BE32(0x80000000); > } > +#endif /* NDEBUG */ This change means outside of debug mode and according to PRM, a zero-length segment is interpreted as containing 2 GiB worth of data, which guarantees some sort of crash. To properly enforce such a limitation, you'd need a check (possibly unlikely()) to reject the packet and stop the TX function at this point anyway. Such a check negates any kind of optimization brought by this commit, as small as it is. You'd better leave the existing code unmodified in my opinion. -- Adrien Mazarguil 6WIND