From: "Xing, Beilei" <beilei.xing@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
Yong Wang <yongwang@vmware.com>,
Olivier Matz <olivier.matz@6wind.com>,
"Wu, Jingjing" <jingjing.wu@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
Julien Meunier <julien.meunier@6wind.com>,
Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
Date: Mon, 20 Jun 2016 12:04:33 +0000 [thread overview]
Message-ID: <94479800C636CB44BD422CB454846E013A3F4F@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB97725836B7346B@irsmsx105.ger.corp.intel.com>
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, June 20, 2016 4:05 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Yong Wang
> <yongwang@vmware.com>; Olivier Matz <olivier.matz@6wind.com>; Wu,
> Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> Monjalon <thomas.monjalon@6wind.com>
> Subject: RE: [dpdk-dev] [PATCH v3] i40e: configure MTU
>
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xing, Beilei
> > Sent: Monday, June 20, 2016 5:50 AM
> > To: Yong Wang; Olivier Matz; Wu, Jingjing
> > Cc: dev@dpdk.org; Julien Meunier; Thomas Monjalon
> > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> >
> >
> > > -----Original Message-----
> > > From: Yong Wang [mailto:yongwang@vmware.com]
> > > Sent: Friday, June 17, 2016 1:40 AM
> > > To: Olivier Matz <olivier.matz@6wind.com>; Xing, Beilei
> > > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> > > Cc: dev@dpdk.org; Julien Meunier <julien.meunier@6wind.com>; Thomas
> > > Monjalon <thomas.monjalon@6wind.com>
> > > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU
> > >
> > > On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz"
> > > <dev-bounces@dpdk.org on behalf of olivier.matz@6wind.com> wrote:
> > >
> > > >Hi Beilei,
> > > >
> > > >On 05/13/2016 10:15 AM, Beilei Xing wrote:
> > > >> This patch enables configuring MTU for i40e.
> > > >> Since changing MTU needs to reconfigure queue, stop port first
> > > >> before configuring MTU.
> > > >>
> > > >> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > > >> ---
> > > >> v3 changes:
> > > >> Add frame size with extra I40E_VLAN_TAG_SIZE.
> > > >> Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> > > >>
> > > >> v2 changes:
> > > >> If mtu is not within the allowed range, return -EINVAL instead of
> -EBUSY.
> > > >> Delete rxq reconfigure cause rxq reconfigure will be finished in
> > > >> i40e_dev_rx_init.
> > > >>
> > > >> drivers/net/i40e/i40e_ethdev.c | 34
> > > >> ++++++++++++++++++++++++++++++++++
> > > >> 1 file changed, 34 insertions(+)
> > > >>
> > > >> [...]
> > > >> +static int
> > > >> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
> > > >> + struct i40e_pf *pf =
> I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> > > >> + struct rte_eth_dev_data *dev_data = pf->dev_data;
> > > >> + uint32_t frame_size = mtu + ETHER_HDR_LEN
> > > >> + + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> > > >> + int ret = 0;
> > > >> +
> > > >> + /* check if mtu is within the allowed range */
> > > >> + if ((mtu < ETHER_MIN_MTU) || (frame_size >
> I40E_FRAME_SIZE_MAX))
> > > >> + return -EINVAL;
> > > >> +
> > > >> + /* mtu setting is forbidden if port is start */
> > > >> + if (dev_data->dev_started) {
> > > >> + PMD_DRV_LOG(ERR,
> > > >> + "port %d must be stopped before configuration\n",
> > > >> + dev_data->port_id);
> > > >> + return -ENOTSUP;
> > > >> + }
> > > >
> > > >I'm not convinced that ENOTSUP is the proper return value here.
> > > >It is usually returned when a function is not implemented, which is
> > > >not the case here: the function is implemented but is forbidden
> > > >because the port is running.
> > > >
> > > >I saw that Julien commented on your v1 that the return value should
> > > >be one of:
> > > > - (0) if successful.
> > > > - (-ENOTSUP) if operation is not supported.
> > > > - (-ENODEV) if *port_id* invalid.
> > > > - (-EINVAL) if *mtu* invalid.
> > > >
> > > >But I think your initial value (-EBUSY) was fine. Maybe it should
> > > >be added in the API instead, with the following description:
> > > > (-EBUSY) if the operation is not allowed when the port is running
> > >
> > > AFAICT, the same check is not done for other drivers that implement
> > > the mac_set op. Wouldn’t it make more sense to have the driver
> > > disable the port, reconfigure and re-enable it in this case, instead
> > > of returning error code? If the consensus in DPDK is to have the
> > > application disable the port first, we need to enforce this policy across all
> devices and clearly document this behavior.
> > >
> > Hi,
> > For ixgbe/igb, there's register for MTU during runtime, but for i40e,
> > setting MTU is finished by firmware during configuration. There'll be some risk
> when disable the port, reconfigure and re-enable the port, so return error code
> in this case currently.
>
> Ok, but then what is the point to introduce mtu_set() for i40e at all?
> Let's just leave it unimplemented, as it is right now.
> Then users would know that for that device they have to do
> dev_stop/dev_configure().
Hi Konstantin,
The original intention is to reduce ops gap in different PMDs, but unfortunately, setting MTU in i40e couldn’t be finished as in ixgbe, so in the function, we change the configuration and note users that if want to configure MTU, stop ports first. The sequence of configuring MTU for i40e is: port stop->set MTU->port start.
We will add according instruction in document.
Beilei
> Konstantin
>
> > Thanks for your comments, we'll improve the document later.
> >
> > > >This would allow the application to take its dispositions to stop
> > > >the port and restart it with the proper jumbo_frame argument.
> > > >
> > > >+CC Thomas which maintains ethdev API.
> > > >
> > > >
> > > >Regards,
> > > >Olivier
next prev parent reply other threads:[~2016-06-20 12:04 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-23 11:26 [dpdk-dev] [PATCH] " Beilei Xing
2016-04-26 2:00 ` Wu, Jingjing
2016-04-27 11:43 ` Julien Meunier
2016-04-27 15:01 ` Xing, Beilei
2016-04-28 3:19 ` [dpdk-dev] [PATCH V2] " Beilei Xing
2016-05-13 8:15 ` [dpdk-dev] [PATCH v3] " Beilei Xing
2016-05-16 12:27 ` Olivier Matz
2016-06-16 17:40 ` Yong Wang
2016-06-16 17:51 ` Yong Wang
2016-06-17 0:03 ` Ananyev, Konstantin
2016-06-20 4:49 ` Xing, Beilei
2016-06-20 4:59 ` Xing, Beilei
2016-06-20 8:05 ` Ananyev, Konstantin
2016-06-20 12:04 ` Xing, Beilei [this message]
2016-06-20 12:15 ` Ananyev, Konstantin
2016-06-20 12:46 ` Xing, Beilei
2016-05-20 15:17 ` [dpdk-dev] [PATCH v4] " Beilei Xing
2016-05-23 1:33 ` Wu, Jingjing
2016-06-09 14:23 ` Bruce Richardson
2016-06-23 10:13 ` Bruce Richardson
2016-06-23 13:37 ` Xing, Beilei
2016-06-23 13:44 ` Bruce Richardson
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=94479800C636CB44BD422CB454846E013A3F4F@SHSMSX101.ccr.corp.intel.com \
--to=beilei.xing@intel.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@intel.com \
--cc=julien.meunier@6wind.com \
--cc=konstantin.ananyev@intel.com \
--cc=olivier.matz@6wind.com \
--cc=thomas.monjalon@6wind.com \
--cc=yongwang@vmware.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).