From: "Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>
To: Shahaf Shuler <shahafs@mellanox.com>, Yongseok Koh <yskoh@mellanox.com>
Cc: Yongseok Koh <yskoh@mellanox.com>,
Adrien Mazarguil <adrien.mazarguil@6wind.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix calculating TSO inline size
Date: Wed, 13 Sep 2017 09:26:06 +0200 [thread overview]
Message-ID: <20170913072606.GA29920@autoinstall.dev.6wind.com> (raw)
In-Reply-To: <VI1PR05MB31498B2482043E7923C9F13FC36E0@VI1PR05MB3149.eurprd05.prod.outlook.com>
On Wed, Sep 13, 2017 at 05:05:14AM +0000, Shahaf Shuler wrote:
> Tuesday, September 12, 2017 9:34 PM, Nélio Laranjeiro:
> > > On Sep 12, 2017, at 12:24 AM, Nélio Laranjeiro
> > >>> Is not it dangerous to assume inl will always be 4 bytes long? Why
> > >>> not writing the real value instead?
> > >> That was for readability of the code and uint32_t will be always
> > >> 4bytes. But for better readability, it should be 'inline_header'
> > >> instead of 'inl' though. I'm also okay with using "4" as well. Which way do
> > you prefer?
> > >
> > > I agree on both, I was not clear enough to explain my thought, if for
> > > some reason the inl moves from uint32_t to uint16_t without touching
> > > the sizeof later, it will cause an issue.
> >
> > I tried to change the sizeof but I found that there are more "sizeof(inl)" in the
> > following lines. Changing all the sizeof would be beyond the scope of this
> > patch. So, how about leaving it as is for consistency?
>
> The inline segment format is not expected to change so easily. It is parsed
> by the HW and HW maintains backward compatibility for all of the WQE
> structures.
We are not talking here about the hardware behavior but about wrong ways to
define hardware values which should be used trough define and not by size of
variable which can be wrongly modified.
If the hardware inline_header size of 4, it should be defined as is.
I won't block this patch as it is not the single place where this sizeof is
used, but it should be replaced as soon as possible by a define to avoid wrong
behaviors in the future.
Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
--
Nélio Laranjeiro
6WIND
next prev parent reply other threads:[~2017-09-13 7:26 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-31 16:27 Yongseok Koh
2017-09-04 14:01 ` Nélio Laranjeiro
2017-09-11 22:17 ` Yongseok Koh
2017-09-12 7:24 ` Nélio Laranjeiro
2017-09-12 18:34 ` Yongseok Koh
2017-09-13 5:05 ` Shahaf Shuler
2017-09-13 7:26 ` Nélio Laranjeiro [this message]
2017-09-15 8:39 ` Ferruh Yigit
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=20170913072606.GA29920@autoinstall.dev.6wind.com \
--to=nelio.laranjeiro@6wind.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=shahafs@mellanox.com \
--cc=stable@dpdk.org \
--cc=yskoh@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).