Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH API-NEXT v1] api: pktio: max frame length #298

Closed
wants to merge 6 commits into from

Conversation

psavol
Copy link
Collaborator

@psavol psavol commented Nov 16, 2017

MTU is not a well defined term for link layer maximum receive and transmit frame sizes. Added functions to request maximum packet input and output frame lengths. Deprecated odp_pktio_mtu() function.

Petri Savolainen added 6 commits November 16, 2017 10:19
Added functions to request maximum packet input and output
frame lengths.

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
Use mtu to implement frame length functions. Various packet
IO devices mtu functions need to be still updated to return
correct frame length values.

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
MTU is not a well defined term for link layer maximum receive
and transmit frame sizes. Use odp_pktin_maxlen() and
odp_pktout_maxlen() instead.

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
Added verbose command line option. When enabled, pktio interface
details are printed after open. This can be used for debugging
interface configuration.

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
Returns maximum ethernet frame size instead of maximum IP
packet MTU.

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
Remove [in] from @param tags as by default parameters
are input only.

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
@muvarov muvarov changed the title api: pktio: max frame length [PATCH API-NEXT v1] api: pktio: max frame length Nov 16, 2017
* @return Maximum frame length at packet input
* @retval 0 on failure
*/
uint32_t odp_pktin_maxlen(odp_pktio_t pktio);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad naming. odp_pktin_xxx functions take argument odp_pktin_queue_t not odp_pktio_t. If you want to keep odp_pktio_t as argument for function, then odp_pktio_capability_t is more logical place for it,

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int odp_pktin_event_queue(odp_pktio_t pktio, odp_queue_t queues[], int num);
int odp_pktin_queue(odp_pktio_t pktio, odp_pktin_queue_t queues[], int num);
uint64_t odp_pktin_wait_time(uint64_t nsec);
uint64_t odp_pktin_ts_res(odp_pktio_t pktio);
odp_time_t odp_pktin_ts_from_ns(odp_pktio_t pktio, uint64_t ns);

These are not the first pktin/pkout functions that have pktio as param. Previously, I considered odp_pktio_rx_xxx() and odp_pktio_tx_xxx() prefixes for these, but odp_pktin_xxx() and odp_pktout_xxx() was more compact and as intuitive.

Also these are not capabilities but the current configured frame lengths. Later on, we'll add configuration options for changing these and capabilities to indicate min/max values. This is now a simple replacement of MTU function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psavol then I'm ok.

Copy link
Contributor

@Bill-Fischofer-Linaro Bill-Fischofer-Linaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the two "extraneous" changes (adding a verbose option on l2fwd and cleaning up the pktio doxygen) I don't see the reason for this renaming.

The term MTU is well established in networking (see RFCs 1191, 4638, 4821) and should already be familiar to anyone working in this area. I don't see what advantage we gain from switching from a standard term to one of our own design.

* @param[in] pktio Packet IO handle.
* @param[in] enable 1 to enable, 0 to disable.
* @param pktio Packet IO handle.
* @param enable 1 to enable, 0 to disable.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a worthwhile cleanup but seems orthogonal to the rest of this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the same API header file. Doing it as part of the patch set avoids potential merge conflicts.

@@ -108,6 +108,7 @@ typedef struct {
int error_check; /**< Check packet errors */
int sched_mode; /**< Scheduler mode */
int num_groups; /**< Number of scheduling groups */
int verbose; /**< Verbose output */
} appl_args_t;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not obvious what these changes have to do with the stated purpose of this PR. This looks like a separate PR altogether.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this option to check the frame sizes. E.g. dpdk pktio returned IP MTU size (1500), not max ethernet frame size (1514). This an easy way to check it on various pktios as l2fwd takes interface as a command line parameter.

@psavol
Copy link
Collaborator Author

psavol commented Nov 20, 2017

Actually, these RFCs refer to various different MTUs and highlight why the change is needed. MTU may refer to maximum IP packet length that can fit into an ethernet frame (e.g. 1500 bytes), or what IP end points have agreed (PMTU) for a link (e.g. 1420 bytes when there's an IPSEC tunnel on the way), or IP packet size that fits into ethernet with PPP headers (1492 bytes), or etc.

Since ODP is not an IP (or any other protocol) stack, it does not maintain IP MTU information. ODP just returns what is currently configured into NIC RX/TX HW registers (pktin_maxlen, pktout_maxlen). Those are the absolute maximums that the HW can handle - no matter what (standard or proprietary) protocols application may be using on top of the link. odp_pktout_maxlen() is the maximum number of bytes that odp_pktout_send() can send out.

@Bill-Fischofer-Linaro
Copy link
Contributor

@Pasvol The term MTU is also protocol-agnostic as you can find it in both Fibre Channel and Infiniband as well as Ethernet-based systems. The difference is that beyond the device MTU there is also the MTUs associated with various protocols. We just need to clarify in the docs that ODP's MTU support is for device MTUs.

@Bill-Fischofer-Linaro
Copy link
Contributor

If @bala-manoharan and @NikhilA-Linaro are OK with this change, I'm OK with it also.

@muvarov
Copy link
Contributor

muvarov commented Nov 24, 2017

@NikhilA-Linaro do you have any update for this?

@psavol
Copy link
Collaborator Author

psavol commented Nov 24, 2017

At least, old Freescale devices had MAXFRM (max frame length) register. In practice, every NIC has max frame registers, but none has MTU registers. So, let's just merge this and move on.

http://elixir.free-electrons.com/linux/v4.2/source/drivers/net/ethernet/freescale/gianfar.c

/* Initialize the max receive frame/buffer lengths */
gfar_write(&regs->maxfrm, priv->rx_buffer_size);
gfar_write(&regs->mrblr, priv->rx_buffer_size);

Copy link
Contributor

@bala-manoharan bala-manoharan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Balasubramanian Manoharan bala.manoharan@linaro.org

@muvarov
Copy link
Contributor

muvarov commented Nov 30, 2017

Merged.

@muvarov muvarov closed this Nov 30, 2017
@psavol psavol deleted the next-frame-len branch January 16, 2018 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants