* Re: [PATCH v2] doc: prog_guide: fix section heading
From: Thomas Monjalon @ 2016-12-06 17:03 UTC (permalink / raw)
To: Baruch Siach; +Cc: dev, John McNamara
In-Reply-To: <869a90aac160d30cd5f0c9fe8e1aec648c39d021.1480484624.git.baruch@tkos.co.il>
2016-11-30 07:43, Baruch Siach:
> This section only deals with Tx queues configuration.
>
> Acked-by: John McNamara <john.mcnamara@intel.com>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Applied (merged with 2 others), thanks
^ permalink raw reply
* Re: [PATCH] doc: remove wrong document description
From: Thomas Monjalon @ 2016-12-06 17:03 UTC (permalink / raw)
To: Baruch Siach; +Cc: dev, Mcnamara, John
In-Reply-To: <B27915DBBA3421428155699D51E4CFE20266ACB5@IRSMSX103.ger.corp.intel.com>
> > The Programmer’s Guide intro is not the Release Notes.
> >
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>
>
> Acked-by: John McNamara <john.mcnamara@intel.com>
Applied (merged with 2 others), thanks
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Ananyev, Konstantin @ 2016-12-06 17:00 UTC (permalink / raw)
To: Nélio Laranjeiro, Morten Brørup
Cc: Wiles, Keith, Richardson, Bruce, dev@dpdk.org, Olivier Matz,
Lu, Wenzhuo, Adrien Mazarguil
In-Reply-To: <20161206162807.GS21794@autoinstall.dev.6wind.com>
> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Tuesday, December 6, 2016 4:28 PM
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: Wiles, Keith <keith.wiles@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Adrien
> Mazarguil <adrien.mazarguil@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types
>
> Hi all,
>
> On Tue, Dec 06, 2016 at 04:34:07PM +0100, Morten Brørup wrote:
> > Hi all,
> >
> > Being a big fan of strong typing, I really like the concept of
> > explicit endian types. Especially if type mismatches can be caught at
> > compile time.
>
> +1,
>
> > However, I think it is too late! That train left the station when the
> > rest of the world - including libraries and headers that might be
> > linked with a DPDK application - decided to use implicit big endian
> > types for network protocols, and has been doing so for decades. And,
> > with all respect, I don't think the DPDK community has the momentum
> > required to change this tradition outside the community.
>
> I don't think, those types can be use from now on to help new API to
> expose explicitly the type they are handling. For older ones, it can
> come in a second step, even if there are not so numerous. Only few of
> them touches the network types.
>
> > Furthermore: If not enforced throughout DPDK (and beyond), it might
> > confuse more than it helps.
>
> The current situation is more confusing, nobody at any layer can rely
> on a precise information, at each function entry we need to verify if
> the callee has already handled the job. The only solution is to browse
> the code to have this information.
>
> Think about any function manipulating network headers (like flow director
> or rte_flow) from the API down to the PMD, it may take a lot of time to
> know at the end if the data is CPU or network ordered, with those types
> it takes less than a second.
Hmm, suppose I have such piece of code:
struct ipv4_hdr iph;
iph.total_length = val0;
iph.packet_id = val1;
...
iph.dst_addr = valn;
hton_whole_ipv4_hdr_at_once(&iph);
How I suppose to indicate via these new types that
hton_whole_ipv4_hdr_at_once() expects ipv4_hdr fields in host byte order?
Other than just putting it into the doc?
Konstantin
^ permalink raw reply
* Re: [PATCH v2 1/3] doc: note that the app list is incomplete
From: Thomas Monjalon @ 2016-12-06 16:53 UTC (permalink / raw)
To: Baruch Siach; +Cc: dev, John McNamara
In-Reply-To: <04aef536b67faf3c09e0f4603736057f95b5ef66.1481027890.git.baruch@tkos.co.il>
2016-12-06 14:38, Baruch Siach:
> Extend the note at the end of Source Organization chapter to also mention
> the app directory.
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
[...]
> --- a/doc/guides/prog_guide/source_org.rst
> +++ b/doc/guides/prog_guide/source_org.rst
> @@ -162,5 +162,5 @@ The examples directory contains sample applications that show how libraries can
>
> .. note::
>
> - The actual examples directory may contain additional sample applications to those shown above.
> + The actual app and examples directories may contain additional applications to those shown above.
> Check the latest DPDK source files for details.
I suggest to remove this file.
Each libraries, drivers and applications are documented somewhere else.
We really must avoid repeating ourself in the documentation and keep
a reasonnably maintainable documentation.
^ permalink raw reply
* Re: [PATCH v2 1/6] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-12-06 16:51 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <1480996340-29871-2-git-send-email-jerin.jacob@caviumnetworks.com>
On Tue, Dec 06, 2016 at 09:22:15AM +0530, Jerin Jacob wrote:
> In a polling model, lcores poll ethdev ports and associated
> rx queues directly to look for packet. In an event driven model,
> by contrast, lcores call the scheduler that selects packets for
> them based on programmer-specified criteria. Eventdev library
> adds support for event driven programming model, which offer
> applications automatic multicore scaling, dynamic load balancing,
> pipelining, packet ingress order maintenance and
> synchronization services to simplify application packet processing.
>
> By introducing event driven programming model, DPDK can support
> both polling and event driven programming models for packet processing,
> and applications are free to choose whatever model
> (or combination of the two) that best suits their needs.
>
> This patch adds the eventdev specification header file.
>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
<snip>
> +
> +/**
> + * The generic *rte_event* structure to hold the event attributes
> + * for dequeue and enqueue operation
> + */
> +struct rte_event {
> + /** WORD0 */
> + RTE_STD_C11
> + union {
> + uint64_t event;
> + /** Event attributes for dequeue or enqueue operation */
> + struct {
> + uint64_t flow_id:20;
> + /**< Targeted flow identifier for the enqueue and
> + * dequeue operation.
> + * The value must be in the range of
> + * [0, nb_event_queue_flows - 1] which
> + * previously supplied to rte_event_dev_configure().
> + */
> + uint64_t sub_event_type:8;
> + /**< Sub-event types based on the event source.
> + * @see RTE_EVENT_TYPE_CPU
> + */
> + uint64_t event_type:4;
> + /**< Event type to classify the event source.
> + * @see RTE_EVENT_TYPE_ETHDEV, (RTE_EVENT_TYPE_*)
> + */
> + uint64_t sched_type:2;
> + /**< Scheduler synchronization type (RTE_SCHED_TYPE_*)
> + * associated with flow id on a given event queue
> + * for the enqueue and dequeue operation.
> + */
> + uint64_t queue_id:8;
> + /**< Targeted event queue identifier for the enqueue or
> + * dequeue operation.
> + * The value must be in the range of
> + * [0, nb_event_queues - 1] which previously supplied to
> + * rte_event_dev_configure().
> + */
> + uint64_t priority:8;
> + /**< Event priority relative to other events in the
> + * event queue. The requested priority should in the
> + * range of [RTE_EVENT_DEV_PRIORITY_HIGHEST,
> + * RTE_EVENT_DEV_PRIORITY_LOWEST].
> + * The implementation shall normalize the requested
> + * priority to supported priority value.
> + * Valid when the device has
> + * RTE_EVENT_DEV_CAP_FLAG_EVENT_QOS capability.
> + */
> + uint64_t op:2;
> + /**< The type of event enqueue operation - new/forward/
> + * etc.This field is not preserved across an instance
> + * and is undefined on dequeue.
> + * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*)
> + */
> + uint64_t impl_opaque:12;
> + /**< Implementation specific opaque value.
> + * An implementation may use this field to hold
> + * implementation specific value to share between
> + * dequeue and enqueue operation.
> + * The application should not modify this field.
> + */
> + };
> + };
> + /** WORD1 */
> + RTE_STD_C11
> + union {
> + uint64_t u64;
> + /**< Opaque 64-bit value */
> + uintptr_t event_ptr;
> + /**< Opaque event pointer */
Since we have a uint64_t member of the union, might this be better as a
void * rather than uintptr_t?
> + struct rte_mbuf *mbuf;
> + /**< mbuf pointer if dequeued event is associated with mbuf */
> + };
> +};
> +
<snip>
> +/**
> + * Link multiple source event queues supplied in *rte_event_queue_link*
> + * structure as *queue_id* to the destination event port designated by its
> + * *port_id* on the event device designated by its *dev_id*.
> + *
> + * The link establishment shall enable the event port *port_id* from
> + * receiving events from the specified event queue *queue_id*
> + *
> + * An event queue may link to one or more event ports.
> + * The number of links can be established from an event queue to event port is
> + * implementation defined.
> + *
> + * Event queue(s) to event port link establishment can be changed at runtime
> + * without re-configuring the device to support scaling and to reduce the
> + * latency of critical work by establishing the link with more event ports
> + * at runtime.
I think this might need to be clarified. The device doesn't need to be
reconfigured, but does it need to be stopped? In SW implementation, this
affects how much we have to make things thread-safe. At minimum I think
we should limit this to having only one thread call the function at a
time, but we may allow enqueue dequeue ops from the data plane to run
in parallel.
> + *
> + * @param dev_id
> + * The identifier of the device.
> + *
> + * @param port_id
> + * Event port identifier to select the destination port to link.
> + *
> + * @param link
> + * Points to an array of *nb_links* objects of type *rte_event_queue_link*
> + * structure which contain the event queue to event port link establishment
> + * attributes.
> + * NULL value is allowed, in which case this function links all the configured
> + * event queues *nb_event_queues* which previously supplied to
> + * rte_event_dev_configure() to the event port *port_id* with normal servicing
> + * priority(RTE_EVENT_DEV_PRIORITY_NORMAL).
> + *
> + * @param nb_links
> + * The number of links to establish
> + *
> + * @return
> + * The number of links actually established. The return value can be less than
> + * the value of the *nb_links* parameter when the implementation has the
> + * limitation on specific queue to port link establishment or if invalid
> + * parameters are specified in a *rte_event_queue_link*.
> + * If the return value is less than *nb_links*, the remaining links at the end
> + * of link[] are not established, and the caller has to take care of them.
> + * If return value is less than *nb_links* then implementation shall update the
> + * rte_errno accordingly, Possible rte_errno values are
> + * (-EDQUOT) Quota exceeded(Application tried to link the queue configured with
> + * RTE_EVENT_QUEUE_CFG_FLAG_SINGLE_LINK to more than one event ports)
> + * (-EINVAL) Invalid parameter
> + *
> + */
> +int
> +rte_event_port_link(uint8_t dev_id, uint8_t port_id,
> + const struct rte_event_queue_link link[],
> + uint16_t nb_links);
> +
> +/**
> + * Unlink multiple source event queues supplied in *queues* from the destination
> + * event port designated by its *port_id* on the event device designated
> + * by its *dev_id*.
> + *
> + * The unlink establishment shall disable the event port *port_id* from
> + * receiving events from the specified event queue *queue_id*
> + *
> + * Event queue(s) to event port unlink establishment can be changed at runtime
> + * without re-configuring the device.
Clarify, as above with link call.
> + *
> + * @param dev_id
> + * The identifier of the device.
> + *
> + * @param port_id
> + * Event port identifier to select the destination port to unlink.
> + *
> + * @param queues
> + * Points to an array of *nb_unlinks* event queues to be unlinked
> + * from the event port.
> + * NULL value is allowed, in which case this function unlinks all the
> + * event queue(s) from the event port *port_id*.
> + *
> + * @param nb_unlinks
> + * The number of unlinks to establish
> + *
> + * @return
> + * The number of unlinks actually established. The return value can be less
> + * than the value of the *nb_unlinks* parameter when the implementation has the
> + * limitation on specific queue to port unlink establishment or
> + * if invalid parameters are specified.
> + * If the return value is less than *nb_unlinks*, the remaining queues at the
> + * end of queues[] are not established, and the caller has to take care of them.
> + * If return value is less than *nb_unlinks* then implementation shall update
> + * the rte_errno accordingly, Possible rte_errno values are
> + * (-EINVAL) Invalid parameter
> + *
> + */
> +int
> +rte_event_port_unlink(uint8_t dev_id, uint8_t port_id,
> + uint8_t queues[], uint16_t nb_unlinks);
> +
<snip>
^ permalink raw reply
* Re: [PATCH v2 0/6] libeventdev API and northbound implementation
From: Bruce Richardson @ 2016-12-06 16:46 UTC (permalink / raw)
To: Jerin Jacob
Cc: dev, thomas.monjalon, hemant.agrawal, gage.eads, harry.van.haaren
In-Reply-To: <1480996340-29871-1-git-send-email-jerin.jacob@caviumnetworks.com>
On Tue, Dec 06, 2016 at 09:22:14AM +0530, Jerin Jacob wrote:
> As previously discussed in RFC v1 [1], RFC v2 [2], with changes
> described in [3] (also pasted below), here is the first non-draft series
> for this new API.
>
> [1] http://dpdk.org/ml/archives/dev/2016-August/045181.html
> [2] http://dpdk.org/ml/archives/dev/2016-October/048592.html
> [3] http://dpdk.org/ml/archives/dev/2016-October/048196.html
>
> v1..v2:
> 1) Remove unnecessary header files from rte_eventdev.h(Thomas)
> 2) Removed PMD driver name(EVENTDEV_NAME_SKELETON_PMD) from rte_eventdev.h(Thomas)
> 3) Removed different #define for different priority schemes. Changed to
> one event device RTE_EVENT_DEV_PRIORITY_* priority (Bruce)
> 4) add const to rte_event_dev_configure(), rte_event_queue_setup(),
> rte_event_port_setup(), rte_event_port_link()(Bruce)
> 5) Fixed missing dev argument in dev->schedule() function(Bruce)
> 6) Changed \see to @see in doxgen comments(Thomas)
> 7) Added additional text in specification to clarify the queue depth(Thomas)
> 8) Changed wait to timeout across the specification(Thomas)
> 9) Added longer explanation for RTE_EVENT_OP_NEW and RTE_EVENT_OP_FORWARD(Thomas)
> 10) Fixed issue with RTE_EVENT_OP_RELEASE doxgen formatting(Thomas)
> 11) Changed to RTE_EVENT_DEV_CFG_FLAG_ from RTE_EVENT_DEV_CFG_(Thomas)
> 12) Changed to EVENT_QUEUE_CFG_FLAG_ from EVENT_QUEUE_CFG_(Thomas)
> 13) s/RTE_EVENT_TYPE_CORE/RTE_EVENT_TYPE_CPU/(Thomas, Gage)
> 14) Removed non burst API and kept only the burst API in the API specification
> (Thomas, Bruce, Harry, Jerin)
> -- Driver interface has non burst API, selection of the non burst API is based
> on num_objects == 1
> 15) sizeeof(struct rte_event) was not 16 in v1. Fixed it in v2
> -- reduced the width of event_type to 4bit to save space for future change
> -- introduced impl_opaque for implementation specific opaque data(Harry),
> Something useful for HW driver too, in the context of removal the need for sepeare
> release API.
> -- squashed other element size and provided enough space to impl_opaque(Jerin)
> -- added RTE_BUILD_BUG_ON(sizeof(struct rte_event) != 16); check
> 16) add union of uint64_t in the second element in struct rte_event to
> make sure the structure has 16byte address all arch(Thomas)
> 17) Fixed invalid check of nb_atomic_order_sequences in implementation(Gage)
> 18) s/EDEV_LOG_ERR/RTE_EDEV_LOG_ERR(Thomas)
> 19) s/rte_eventdev_pmd_/rte_event_pmd_/(Bruce)
> 20) added fine details of distributed vs centralized scheduling information
> in the specification and introduced RTE_EVENT_DEV_CAP_FLAG_DISTRIBUTED_SCHED
> flag(Gage)
> 21)s/RTE_EVENT_QUEUE_CFG_FLAG_SINGLE_CONSUMER/RTE_EVENT_QUEUE_CFG_FLAG_SINGLE_LINK (Jerin)
> to remove the confusion to between another producer and consumer in sw eventdev driver
> 22) Northbound api implementation patch spited to more logical patches(Thomas)
>
>
Thanks for this Jerin, great job tracking the changes between the
versions.
Couple of comments I have to make on the patches thus far, but I think
we are near having a first version we can commit to a next-event tree.
/Bruce
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Richardson, Bruce @ 2016-12-06 16:36 UTC (permalink / raw)
To: Wiles, Keith, Nélio Laranjeiro
Cc: Morten Brørup, Ananyev, Konstantin, DPDK, Olivier Matz,
Lu, Wenzhuo, Adrien Mazarguil
In-Reply-To: <D1B9E864-EE9F-424D-8EE4-D62AC3B4FE90@intel.com>
> -----Original Message-----
> From: Wiles, Keith
> Sent: Tuesday, December 6, 2016 4:32 PM
> To: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>
> Cc: Morten Brørup <mb@smartsharesystems.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; DPDK <dev@dpdk.org>; Olivier Matz
> <olivier.matz@6wind.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Adrien
> Mazarguil <adrien.mazarguil@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types
>
>
> > On Dec 6, 2016, at 10:28 AM, Nélio Laranjeiro
> <nelio.laranjeiro@6wind.com> wrote:
> >
> > Hi all,
> >
> > On Tue, Dec 06, 2016 at 04:34:07PM +0100, Morten Brørup wrote:
> >> Hi all,
> >>
> >> Being a big fan of strong typing, I really like the concept of
> >> explicit endian types. Especially if type mismatches can be caught at
> >> compile time.
> >
> > +1,
> >
> >> However, I think it is too late! That train left the station when the
> >> rest of the world - including libraries and headers that might be
> >> linked with a DPDK application - decided to use implicit big endian
> >> types for network protocols, and has been doing so for decades. And,
> >> with all respect, I don't think the DPDK community has the momentum
> >> required to change this tradition outside the community.
> >
> > I don't think, those types can be use from now on to help new API to
> > expose explicitly the type they are handling. For older ones, it can
> > come in a second step, even if there are not so numerous. Only few of
> > them touches the network types.
> >
> >> Furthermore: If not enforced throughout DPDK (and beyond), it might
> >> confuse more than it helps.
> >
> > The current situation is more confusing, nobody at any layer can rely
> > on a precise information, at each function entry we need to verify if
> > the callee has already handled the job. The only solution is to
> > browse the code to have this information.
> >
> > Think about any function manipulating network headers (like flow
> > director or rte_flow) from the API down to the PMD, it may take a lot
> > of time to know at the end if the data is CPU or network ordered, with
> > those types it takes less than a second.
>
> Still Documentation should handle this problem without code and ABI
> changes.
>
While my additional suggestion of compiler-enforced endian correctness may break the ABI (though even that is not certain since parameters would be the same size, just from a compiler syntax analysis side the result would be different, I think), if I'm reading it correctly, Neilo's original suggestion of just using typedefs for big or little endian won't affect the ABI as the underlying types are exactly the same as before. Only the type name has changed to document for the user the endianness the expected data is to be. In effect, the original suggestion is a documentation patch - just the code is the documentation.
Regards,
/Bruce
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Wiles, Keith @ 2016-12-06 16:31 UTC (permalink / raw)
To: Nélio Laranjeiro
Cc: Morten Brørup, Ananyev, Konstantin, Richardson, Bruce, DPDK,
Olivier Matz, Lu, Wenzhuo, Adrien Mazarguil
In-Reply-To: <20161206162807.GS21794@autoinstall.dev.6wind.com>
> On Dec 6, 2016, at 10:28 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
>
> Hi all,
>
> On Tue, Dec 06, 2016 at 04:34:07PM +0100, Morten Brørup wrote:
>> Hi all,
>>
>> Being a big fan of strong typing, I really like the concept of
>> explicit endian types. Especially if type mismatches can be caught at
>> compile time.
>
> +1,
>
>> However, I think it is too late! That train left the station when the
>> rest of the world - including libraries and headers that might be
>> linked with a DPDK application - decided to use implicit big endian
>> types for network protocols, and has been doing so for decades. And,
>> with all respect, I don't think the DPDK community has the momentum
>> required to change this tradition outside the community.
>
> I don't think, those types can be use from now on to help new API to
> expose explicitly the type they are handling. For older ones, it can
> come in a second step, even if there are not so numerous. Only few of
> them touches the network types.
>
>> Furthermore: If not enforced throughout DPDK (and beyond), it might
>> confuse more than it helps.
>
> The current situation is more confusing, nobody at any layer can rely
> on a precise information, at each function entry we need to verify if
> the callee has already handled the job. The only solution is to browse
> the code to have this information.
>
> Think about any function manipulating network headers (like flow director
> or rte_flow) from the API down to the PMD, it may take a lot of time to
> know at the end if the data is CPU or network ordered, with those types
> it takes less than a second.
Still Documentation should handle this problem without code and ABI changes.
>
> Regards,
>
> --
> Nélio Laranjeiro
> 6WIND
Regards,
Keith
^ permalink raw reply
* Re: [PATCH v1] doc: fix relative path of Nic table input file
From: Thomas Monjalon @ 2016-12-06 16:31 UTC (permalink / raw)
To: Mcnamara, John; +Cc: dev, De Lara Guarch, Pablo, stable
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D89747694A23@IRSMSX108.ger.corp.intel.com>
> > Fix relative path between sphinx conf.py file and Nic table file
> > to allow automatic build on ReadTheDocs.
> >
> > Fixes: 9db3f52126fb ("doc: generate NIC overview table from ini files")
> >
> > Signed-off-by: John McNamara <john.mcnamara@intel.com>
>
> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Nélio Laranjeiro @ 2016-12-06 16:28 UTC (permalink / raw)
To: Morten Brørup
Cc: Wiles, Keith, Ananyev, Konstantin, Richardson, Bruce, dev,
Olivier Matz, Lu, Wenzhuo, Adrien Mazarguil
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EAA47@smartserver.smartshare.dk>
Hi all,
On Tue, Dec 06, 2016 at 04:34:07PM +0100, Morten Brørup wrote:
> Hi all,
>
> Being a big fan of strong typing, I really like the concept of
> explicit endian types. Especially if type mismatches can be caught at
> compile time.
+1,
> However, I think it is too late! That train left the station when the
> rest of the world - including libraries and headers that might be
> linked with a DPDK application - decided to use implicit big endian
> types for network protocols, and has been doing so for decades. And,
> with all respect, I don't think the DPDK community has the momentum
> required to change this tradition outside the community.
I don't think, those types can be use from now on to help new API to
expose explicitly the type they are handling. For older ones, it can
come in a second step, even if there are not so numerous. Only few of
them touches the network types.
> Furthermore: If not enforced throughout DPDK (and beyond), it might
> confuse more than it helps.
The current situation is more confusing, nobody at any layer can rely
on a precise information, at each function entry we need to verify if
the callee has already handled the job. The only solution is to browse
the code to have this information.
Think about any function manipulating network headers (like flow director
or rte_flow) from the API down to the PMD, it may take a lot of time to
know at the end if the data is CPU or network ordered, with those types
it takes less than a second.
Regards,
--
Nélio Laranjeiro
6WIND
^ permalink raw reply
* Re: [PATCH] nfp: add doc about supported features
From: Ferruh Yigit @ 2016-12-06 16:01 UTC (permalink / raw)
To: Alejandro Lucero, Mcnamara, John; +Cc: dev@dpdk.org
In-Reply-To: <CAD+H99260W3EVNH7XX-URF=NV3C7=jH0veAbprz5m=3V6m1fVA@mail.gmail.com>
On 12/6/2016 2:51 PM, Alejandro Lucero wrote:
> Then I guess I should send another version of this patch.
I can also update the patch while applying, if you agree with following
content (simply just all =N removed):
[Features]
+SR-IOV = Y
+Link status = Y
+Link status event = Y
+Queue start/stop = Y
+MTU update = Y
+Jumbo frame = Y
+RSS hash = Y
+RSS key update = Y
+RSS reta update = Y
+Flow control = Y
+VLAN offload = Y
+L3 checksum offload = Y
+L4 checksum offload = Y
+Promiscuous mode = Y
+Basic stats = Y
+Stats per queue = Y
+Linux UIO = Y
+Linux VFIO = Y
+x86-64 = Y
+Usage doc = Y
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Ferruh Yigit @ 2016-12-06 15:53 UTC (permalink / raw)
To: Thomas Monjalon, dev, Jan Medala, Jakub Palider, Netanel Belgazal,
Evgeny Schemeilin, Alejandro Lucero, Yuanhan Liu, Yong Wang,
Andrew Rybchenko, Hemant Agrawal
Cc: Tomasz Kulasek, konstantin.ananyev
In-Reply-To: <8317180.L80Qf11uiu@xps13>
On 11/28/2016 11:03 AM, Thomas Monjalon wrote:
> We need attention of every PMD developers on this thread.
>
> Reminder of what Konstantin suggested:
> "
> - if the PMD supports TX offloads AND
> - if to be able use any of these offloads the upper layer SW would have to:
> * modify the contents of the packet OR
> * obey HW specific restrictions
> then it is a PMD developer responsibility to provide tx_prep() that would implement
> expected modifications of the packet contents and restriction checks.
> Otherwise, tx_prep() implementation is not required and can be safely set to NULL.
> "
>
> I copy/paste also my previous conclusion:
>
> Before txprep, there is only one API: the application must prepare the
> packets checksum itself (get_psd_sum in testpmd).
> With txprep, the application have 2 choices: keep doing the job itself
> or call txprep which calls a PMD-specific function.
> The question is: does non-Intel drivers need a checksum preparation for TSO?
> Will it behave well if txprep does nothing in these drivers?
>
> When looking at the code, most of drivers handle the TSO flags.
> But it is hard to know whether they rely on the pseudo checksum or not.
>
> git grep -l 'PKT_TX_UDP_CKSUM\|PKT_TX_TCP_CKSUM\|PKT_TX_TCP_SEG' drivers/net/
>
> drivers/net/bnxt/bnxt_txr.c
> drivers/net/cxgbe/sge.c
> drivers/net/e1000/em_rxtx.c
> drivers/net/e1000/igb_rxtx.c
> drivers/net/ena/ena_ethdev.c
> drivers/net/enic/enic_rxtx.c
> drivers/net/fm10k/fm10k_rxtx.c
> drivers/net/i40e/i40e_rxtx.c
> drivers/net/ixgbe/ixgbe_rxtx.c
> drivers/net/mlx4/mlx4.c
> drivers/net/mlx5/mlx5_rxtx.c
> drivers/net/nfp/nfp_net.c
> drivers/net/qede/qede_rxtx.c
> drivers/net/thunderx/nicvf_rxtx.c
> drivers/net/virtio/virtio_rxtx.c
> drivers/net/vmxnet3/vmxnet3_rxtx.c
>
> Please, we need a comment for each driver saying
> "it is OK, we do not need any checksum preparation for TSO"
> or
> "yes we have to implement tx_prepare or TSO will not work in this mode"
>
Still waiting response from PMDs:
- ena
- nfp
- virtio
Waiting clarification for preparation requirements:
- vmxnet3
Also including new PMDs to the thread:
- sfc
- dpaa2
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Morten Brørup @ 2016-12-06 15:34 UTC (permalink / raw)
To: Wiles, Keith, Ananyev, Konstantin
Cc: Richardson, Bruce, Nélio Laranjeiro, dev, Olivier Matz,
Lu, Wenzhuo, Adrien Mazarguil
In-Reply-To: <7E1BBBA1-F384-4235-8A82-4B0D6DC0889C@intel.com>
Hi all,
Being a big fan of strong typing, I really like the concept of explicit endian types. Especially if type mismatches can be caught at compile time.
However, I think it is too late! That train left the station when the rest of the world - including libraries and headers that might be linked with a DPDK application - decided to use implicit big endian types for network protocols, and has been doing so for decades. And, with all respect, I don't think the DPDK community has the momentum required to change this tradition outside the community.
Furthermore: If not enforced throughout DPDK (and beyond), it might confuse more than it helps.
Med venlig hilsen / kind regards
- Morten Brørup
^ permalink raw reply
* Re: [PATCH] dpdk: Fix abort on double free.
From: Aaron Conole @ 2016-12-06 15:25 UTC (permalink / raw)
To: Daniele Di Proietto
Cc: Ilya Maximets, dev@openvswitch.org, Dyasly Sergey, Heetae Ahn,
dev@dpdk.org, Kevin Traynor, Mauricio Vásquez
In-Reply-To: <51C98877-0199-473A-AB2B-248B7874B62F@vmware.com>
Daniele Di Proietto <diproiettod@vmware.com> writes:
> On 29/11/2016 07:57, "Aaron Conole" <aconole@redhat.com> wrote:
>
>>Hi Ilya,
>>
>>Ilya Maximets <i.maximets@samsung.com> writes:
>>
>>> On 28.11.2016 21:55, Aaron Conole wrote:
>>>> Ilya Maximets <i.maximets@samsung.com> writes:
>>>>
>>>>> According to DPDK API (lib/librte_eal/common/include/rte_eal.h):
>>>>>
>>>>> "After the call to rte_eal_init(), all arguments argv[x]
>>>>> with x < ret may be modified and should not be accessed
>>>>> by the application."
>>>>>
>>>>> This means, that OVS must not free the arguments passed to DPDK.
>>>>> In real world, 'rte_eal_init()' replaces the last argument in
>>>>> 'dpdk_argv' with the first one by doing this:
>>>>
>>>> Thanks for spotting this error, Ilya.
>>>>
>>>>> # eal_parse_args() from lib/librte_eal/linuxapp/eal/eal.c
>>>>>
>>>>> char *prgname = argv[0];
>>>>> ...
>>>>> if (optind >= 0)
>>>>> argv[optind-1] = prgname;
>>>>>
>>>>> This leads to double free inside 'deferred_argv_release()' and
>>>>> possible ABORT at exit:
>>>>
>>>> I haven't seen this, which is both shocking and scary - the commit which
>>>> does this copy is almost 4 years old; did you have to do anything
>>>> specific for this behavior to occur? Did something change in DPDK
>>>> recently that exposed this behavior? Just wondering how you reproduced
>>>> it.
>>>
>>> Abort was caught up accidentally. I'm able to reproduce it on my a
>>> little unusual testing system (ARMv8 + Fedora 21 + clang 3.5) without
>>> any specific manipulations. The bug exists always but it's hard
>>> for libc to detect double free here because there are many other
>>> frees/allocations at exit time. I've used following patch to confirm
>>> the issue if it wasn't detected by libc:
>>
>>Well, it's at least good that you can observe it consistently. Did you
>>try my provided patch to see if that works as well?
>>
>>> diff --git a/lib/dpdk.c b/lib/dpdk.c
>>> index 49a589a..65d2d28 100644
>>> --- a/lib/dpdk.c
>>> +++ b/lib/dpdk.c
>>> @@ -258,6 +258,8 @@ deferred_argv_release(void)
>>> {
>>> int result;
>>> for (result = 0; result < dpdk_argc; ++result) {
>>> + VLOG_INFO("DPDK ARGV release: %2d: 0x%" PRIx64 " (%s)",
>>> + result, (intptr_t)dpdk_argv[result], dpdk_argv[result]);
>>> free(dpdk_argv[result]);
>>> }
>>>
>>
>>It's quite glaring after studying the code. Really good catch!
>
> I agree, thanks for spotting this
>
>>
>>>>
>>>>> *** Error in `ovs-vswitchd': double free or corruption (fasttop) <...> ***
>>>>> Program received signal SIGABRT, Aborted.
>>>>>
>>>>> #0 raise () from /lib64/libc.so.6
>>>>> #1 abort () from /lib64/libc.so.6
>>>>> #2 __libc_message () from /lib64/libc.so.6
>>>>> #3 free () from /lib64/libc.so.6
>>>>> #4 deferred_argv_release () at lib/dpdk.c:261
>>>>> #5 __run_exit_handlers () from /lib64/libc.so.6
>>>>> #6 exit () from /lib64/libc.so.6
>>>>> #7 __libc_start_main () from /lib64/libc.so.6
>>>>> #8 _start ()
>>>>>
>>>>> Fix that by not calling free for the memory passed to DPDK.
>>>>>
>>>>> CC: Aaron Conole <aconole@redhat.com>
>>>>> Fixes: bab694097133 ("netdev-dpdk: Convert initialization from
>>>>> cmdline to db")
>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>> ---
>>>>
>>>> We need to free the memory - I think that is not a question;
>>>
>>> Actually, it is. According to DPDK API (see above) 'rte_eal_init()'
>>> takes the ownership of 'argv'. This means that we must not free
>>> or use this memory.
>>
>>Apologies for the ranty-wall of text below.
>>
>>DPDK *cannot* take ownership of freeing this memory, unless 1) it expects a
>>completely separate array from argv/argc than the one passed during
>>program execution and initialization, or 2) it expects the hosted
>>environment to give it the responsibility of cleaning this up. It
>>explicitly claims that the argv/argc is what comes from main(), and
>>therefore should obey the restrictions and privileges afforded those
>>variables.
>>
>>In fact, I don't even see anywhere that dpdk preserves argv, *at all*.
>>Looking through the history very quickly (admittedly just back to commit
>>af75078fece3615088e561357c1e97603e43a5fe in dpdk) confirms that dpdk
>>hasn't stored the arguments anywhere to do any processing.
>>
>>DPDK api guide does NOT state that it takes possession - and that matches
>>with what happens in the code, BUT I will agree the statement
>>
>> 'all arguments argv[x] with x < ret may be modified and should not be
>> accessed by the application'
>>
>>is a bit ambiguous. I think it's trying to say that the application should do
>>its getopt()s parsing before calling the dpdk init routine, because DPDK libs
>>will change the array. I don't see a reason for modifying the array in
>>the code (the `argv[optind-1] = progname`), but if the dpdk library wants
>>to do that, it is free to do so according to C99 5.1.2.2.1; I think
>>it's best we always free what we allocate, which is why I suggested the
>>side array patch which stores additional pointers to the data to be
>>free'd up at exit.
>>
>>I am not sure which is more appropriate, since this is an exit condition,
>>after all. The memory will get free()d up eventually by the
>>environments on which OvS runs. It doesn't _feel_ correct to leave the
>>memory dangling, since we can free it.
>>
>>Anyone else have thoughts on this?
>
> I don't think it's a big deal to leak memory that has to be used until the process
> terminates. There are other examples of this in OvS, such as 'timewarp_seq' in
> lib/timeval.c. They should be reported by valgrind as "still reachable".
With Ilya's incremental, it won't - we will drop the static variables
which would still hold references, as well. So valgrind will report it
as leaked for sure.
> That said, at some point we might want to have 100% leak free valgrind runs, so
> I think it's be better to free everything we allocate, so I would prefer Aaron's
> solution. I don't think DPDK should expect the arguments to be available in exit
> handlers, i.e. after main() returns.
>
> I don't feel strongly about it though, since, if I'm not mistaken, valgrind doesn't
> support DPDK yet.
I'm okay if you want to take Ilya's patches for this, since I can't
truly test for this case (my environment hasn't reproduced the report).
Alternately, I can put in some printf() calls and prove it out that way,
then submit my patch formally. I would prefer if OvS would release the
memory, since it isn't required.
On further thought, though, I actually think it should be released
immediately, and you can see my original proposal did that, and was told
to change:
https://mail.openvswitch.org/pipermail/ovs-dev/2015-December/306540.html
So, I'd much prefer a patch which frees immediately after calling
dpdk_init. Nothing in the dpdk API documentation makes me believe that
it expects to use the memory.
Does anyone object to me submitting a formal patch which does an
immediate free of the memory passed to 'dpdk_init()' as the 'argv'
parameter?
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Wiles, Keith @ 2016-12-06 14:56 UTC (permalink / raw)
To: Ananyev, Konstantin
Cc: Richardson, Bruce, Nélio Laranjeiro, dev@dpdk.org,
Olivier Matz, Lu, Wenzhuo, Adrien Mazarguil
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0E47E1@irsmsx105.ger.corp.intel.com>
> On Dec 6, 2016, at 8:45 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
>
>
>
>>
>> On Tue, Dec 06, 2016 at 12:41:00PM +0000, Ananyev, Konstantin wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Richardson, Bruce
>>>> Sent: Tuesday, December 6, 2016 11:55 AM
>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>>>> Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>; Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>> Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types
>>>>
>>>> On Tue, Dec 06, 2016 at 11:23:42AM +0000, Ananyev, Konstantin wrote:
>>>>> Hi Neilo,
>>>>>
>>>>>
>>>>> Hi Neilo,
>>>>>>>>
>>>>>>>> This commit introduces new rte_{le,be}{16,32,64}_t types and updates
>>>>>>>> rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
>>>>>>>> accordingly.
>>>>>>>>
>>>>>>>> Specific big/little endian types avoid uncertainty and conversion mistakes.
>>>>>>>>
>>>>>>>> No ABI change since these are simply typedefs to the original types.
>>>>>>>
>>>>>>> It seems like quite a lot of changes...
>>>>>>> Could you probably explain what will be the benefit in return?
>>>>>>> Konstantin
>>>>>>
>>>>>> Hi Konstantin,
>>>>>>
>>>>>> The benefit is to provide documented byte ordering for data types
>>>>>> software is manipulating to determine when network to CPU (or CPU to
>>>>>> network) conversion must be performed.
>>>>>
>>>>> Ok, but is it really worth it?
>>>>> User can still make a mistake and forget to call ntoh()/hton() at some particular place.
>>>>> From other side most people do know that network protocols headers are usually in BE format.
>>>>> I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis
>>>>> based on these special types or so.
>>>>> Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs
>>>>> (and might be in some others too) to be consistent?
>>>>> Konstantin
>>>>>
>>>>
>>>> I actually quite like this patch as I think it will help make things
>>>> clear when the user is possibly doing something wrong. I don't think we
>>>> need to globally change all PMDs to use the types, though.
>>>
>>> Ok, so where do you believe we should draw a line?
>>> Why let say inside lib/librte_net people should use these typedefs, but
>>> inside drivers/net/ixgbe they don't?
>>
>> Because those are not public APIs. It would be great if driver writers
>> used the typedefs, but I don't think it should be mandatory.
>
> Ok, so only public API would have to use these typedefs when appropriate, correct?
> I still think that the effort to make these changes and keep this rule outweighs the benefit,
> but ok if everyone else think it is useful - I wouldn't object too much.
I believe the effort and advantages to this change have no real benefit when you can document the type in the function header. Adding a structure around the simple type just adds more typing and still will be difficult to manage even if it gives some compiler checking. The change would not prevent someone putting a BE value into a LE variable, right?
I would not like to see this type of change when documentation would be enough here. Breaking the ABI is a big thing here for a large number of APIs. We keep breaking the ABI and we need to stop doing it on every release of DPDK.
>
>>
>>>
>>>>
>>>> One thing I'm wondering though, is if we might want to take this
>>>> further. For little endian environments, we could define the big endian
>>>> types as structs using typedefs, and similarly the le types on be
>>>> platforms, so that assigning from the non-native type to the native one
>>>> without a transformation function would cause a compiler error.
>>>
>>> Not sure I understand you here.
>>> Could you possibly provide some example?
>>>
>> typedef struct {
>> short val;
>> } rte_be16_t;
>
> Hmm, so:
> uint32_t x = rte_be_to_cpu_32(1);
> would suddenly stop compiling?
> That definitely looks like an ABI breakage to me.
> Konstantin
>
>>
>> That way if you try to assign a value of type rte_be16_t to a uint16_t
>> variable you'll get a compiler error, unless you use an appropriate
>> conversion function. In short, it changes things from not just looking
>> wrong - which is the main purpose of Neilo's patchset - to actually
>> making it incorrect from the compiler's point of view too.
>>
>> /Bruce
Regards,
Keith
^ permalink raw reply
* Re: [PATCH] nfp: add doc about supported features
From: Alejandro Lucero @ 2016-12-06 14:51 UTC (permalink / raw)
To: Mcnamara, John; +Cc: Yigit, Ferruh, dev@dpdk.org
In-Reply-To: <B27915DBBA3421428155699D51E4CFE20266F604@IRSMSX103.ger.corp.intel.com>
Then I guess I should send another version of this patch.
On Tue, Dec 6, 2016 at 12:46 PM, Mcnamara, John <john.mcnamara@intel.com>
wrote:
> > -----Original Message-----
> > From: Yigit, Ferruh
> > Sent: Tuesday, December 6, 2016 12:08 PM
> > To: Alejandro Lucero <alejandro.lucero@netronome.com>; dev@dpdk.org;
> > Mcnamara, John <john.mcnamara@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] nfp: add doc about supported features
> >
> > On 12/2/2016 8:17 AM, Alejandro Lucero wrote:
> > > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > > ---
> > > doc/guides/nics/features/nfp.ini | 25 +++++++++++++++++++++++++
> > > 1 file changed, 25 insertions(+)
> > >
> > > diff --git a/doc/guides/nics/features/nfp.ini
> > b/doc/guides/nics/features/nfp.ini
> > > index d967151..dd78233 100644
> > > --- a/doc/guides/nics/features/nfp.ini
> > > +++ b/doc/guides/nics/features/nfp.ini
> > > @@ -4,3 +4,28 @@
> > > ; Refer to default.ini for the full list of available PMD features.
> > > ;
> > > [Features]
> > > +SR-IOV = Y
> > > +Link status = Y
> > > +Link status event = Y
> > > +Rx interrupt = N
> >
> > Hi John,
> >
> > Do we support 'N'? Since this is the first 'N' in the table.
> >
> > I guess not having a value implies feature is not supported.
>
> N or any other single letter is supported.
>
> The nice thing about it is that it is explicitly NO rather than implicitly
> NO. However, if all the nics filled in the NOes then the table might be
> hard to read.
>
> So, for consistency it is probably best to just let the unsupported
> features be blank.
>
>
>
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Ananyev, Konstantin @ 2016-12-06 14:45 UTC (permalink / raw)
To: Richardson, Bruce
Cc: Nélio Laranjeiro, dev@dpdk.org, Olivier Matz, Lu, Wenzhuo,
Adrien Mazarguil
In-Reply-To: <20161206133427.GB15416@bricha3-MOBL3.ger.corp.intel.com>
>
> On Tue, Dec 06, 2016 at 12:41:00PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Richardson, Bruce
> > > Sent: Tuesday, December 6, 2016 11:55 AM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>; Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types
> > >
> > > On Tue, Dec 06, 2016 at 11:23:42AM +0000, Ananyev, Konstantin wrote:
> > > > Hi Neilo,
> > > >
> > > >
> > > > Hi Neilo,
> > > > > > >
> > > > > > > This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> > > > > > > rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> > > > > > > accordingly.
> > > > > > >
> > > > > > > Specific big/little endian types avoid uncertainty and conversion mistakes.
> > > > > > >
> > > > > > > No ABI change since these are simply typedefs to the original types.
> > > > > >
> > > > > > It seems like quite a lot of changes...
> > > > > > Could you probably explain what will be the benefit in return?
> > > > > > Konstantin
> > > > >
> > > > > Hi Konstantin,
> > > > >
> > > > > The benefit is to provide documented byte ordering for data types
> > > > > software is manipulating to determine when network to CPU (or CPU to
> > > > > network) conversion must be performed.
> > > >
> > > > Ok, but is it really worth it?
> > > > User can still make a mistake and forget to call ntoh()/hton() at some particular place.
> > > > From other side most people do know that network protocols headers are usually in BE format.
> > > > I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis
> > > > based on these special types or so.
> > > > Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs
> > > > (and might be in some others too) to be consistent?
> > > > Konstantin
> > > >
> > >
> > > I actually quite like this patch as I think it will help make things
> > > clear when the user is possibly doing something wrong. I don't think we
> > > need to globally change all PMDs to use the types, though.
> >
> > Ok, so where do you believe we should draw a line?
> > Why let say inside lib/librte_net people should use these typedefs, but
> > inside drivers/net/ixgbe they don't?
>
> Because those are not public APIs. It would be great if driver writers
> used the typedefs, but I don't think it should be mandatory.
Ok, so only public API would have to use these typedefs when appropriate, correct?
I still think that the effort to make these changes and keep this rule outweighs the benefit,
but ok if everyone else think it is useful - I wouldn't object too much.
>
> >
> > >
> > > One thing I'm wondering though, is if we might want to take this
> > > further. For little endian environments, we could define the big endian
> > > types as structs using typedefs, and similarly the le types on be
> > > platforms, so that assigning from the non-native type to the native one
> > > without a transformation function would cause a compiler error.
> >
> > Not sure I understand you here.
> > Could you possibly provide some example?
> >
> typedef struct {
> short val;
> } rte_be16_t;
Hmm, so:
uint32_t x = rte_be_to_cpu_32(1);
would suddenly stop compiling?
That definitely looks like an ABI breakage to me.
Konstantin
>
> That way if you try to assign a value of type rte_be16_t to a uint16_t
> variable you'll get a compiler error, unless you use an appropriate
> conversion function. In short, it changes things from not just looking
> wrong - which is the main purpose of Neilo's patchset - to actually
> making it incorrect from the compiler's point of view too.
>
> /Bruce
^ permalink raw reply
* Re: [PATCH v2] doc: add pdump library to API doxygen
From: Thomas Monjalon @ 2016-12-06 14:44 UTC (permalink / raw)
To: Pattan, Reshma; +Cc: dev, Mcnamara, John, Yigit, Ferruh
In-Reply-To: <B27915DBBA3421428155699D51E4CFE20266C2B8@IRSMSX103.ger.corp.intel.com>
2016-12-01 13:28, Mcnamara, John:
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > Sent: Thursday, December 1, 2016 11:02 AM
> > To: dev@dpdk.org
> > Cc: Pattan, Reshma <reshma.pattan@intel.com>
> > Subject: [dpdk-dev] [PATCH v2] doc: add pdump library to API doxygen
> >
> > From: Reshma Pattan <reshma.pattan@intel.com>
> >
> > Add pdump library to API doxygen.
> >
> > Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
>
> Acked-by: John McNamara <john.mcnamara@intel.com>
It triggers a doxygen error:
lib/librte_pdump/rte_pdump.h:204: warning: Found unknown command `\type'
lib/librte_pdump/rte_pdump.h:214: warning: The following parameters of
rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type)
are not documented:
parameter 'type'
Fixed with this change:
- * @type
+ * @param type
Applied, thanks
^ permalink raw reply
* Re: [PATCH] doc: remove the repeat words in documentation.rst
From: Thomas Monjalon @ 2016-12-06 14:38 UTC (permalink / raw)
To: Yong Wang; +Cc: dev, Mcnamara, John
In-Reply-To: <B27915DBBA3421428155699D51E4CFE20266F47E@IRSMSX103.ger.corp.intel.com>
> > Signed-off-by: Yong Wang <wang.yong19@zte.com.cn>
>
> Good catch.
>
>
> Acked-by: John McNamara <john.mcnamara@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] doc: fix typos in code comments
From: Thomas Monjalon @ 2016-12-06 14:33 UTC (permalink / raw)
To: Yong Wang; +Cc: dev, Mcnamara, John, olivier.matz
In-Reply-To: <B27915DBBA3421428155699D51E4CFE20266AB9A@IRSMSX103.ger.corp.intel.com>
> > Signed-off-by: Yong Wang <wang.yong19@zte.com.cn>
>
> Acked-by: John McNamara <john.mcnamara@intel.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH v3] mempool: remove a redundant word "for" in comment
From: Thomas Monjalon @ 2016-12-06 14:18 UTC (permalink / raw)
To: Wei Zhao; +Cc: dev, olivier.matz
In-Reply-To: <1480909200-34647-1-git-send-email-wei.zhao1@intel.com>
2016-12-05 11:40, Wei Zhao:
> There is a redundant repetition word "for" in commnet line of the
> file rte_mempool.h after the definition of RTE_MEMPOOL_OPS_NAMESIZE.
> The word "for"appear twice in line 359 and 360. One of them is
> redundant, so delete it.
>
> Fixes: 449c49b93a6b ("mempool: support handler operations")
>
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Acked-by: John McNamara <john.mcnamara@intel.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
Applied (with small typos fixed in message), thanks
^ permalink raw reply
* Re: [PATCH v3] mempool: remove a redundant socket id assignment
From: Thomas Monjalon @ 2016-12-06 14:13 UTC (permalink / raw)
To: Wei Zhao; +Cc: dev, olivier.matz
In-Reply-To: <1480909144-34544-1-git-send-email-wei.zhao1@intel.com>
2016-12-05 11:39, Wei Zhao:
> There is a redundant repetition mempool socket_id assignment in the
> file rte_mempool.c in function rte_mempool_create_empty. The
> statement "mp->socket_id = socket_id;"appear twice in line 821
> and 824. One of them is redundant, so delete it.
>
> Fixes: 85226f9c526b ("mempool: introduce a function to create an empty pool")
>
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Acked-by: John McNamara <john.mcnamara@intel.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
Applied, thanks
^ permalink raw reply
* Re: [PATCH] net: introduce big and little endian types
From: Wiles, Keith @ 2016-12-06 14:06 UTC (permalink / raw)
To: Nélio Laranjeiro
Cc: Ananyev, Konstantin, dev@dpdk.org, Olivier Matz, Lu, Wenzhuo,
Adrien Mazarguil
In-Reply-To: <20161205120603.GL21794@autoinstall.dev.6wind.com>
> On Dec 5, 2016, at 6:06 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
>
> On Mon, Dec 05, 2016 at 10:09:05AM +0000, Ananyev, Konstantin wrote:
>> Hi Neilo,
>>
>>>
>>> This commit introduces new rte_{le,be}{16,32,64}_t types and updates
>>> rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
>>> accordingly.
>>>
>>> Specific big/little endian types avoid uncertainty and conversion mistakes.
>>>
>>> No ABI change since these are simply typedefs to the original types.
>>
>> It seems like quite a lot of changes...
>> Could you probably explain what will be the benefit in return?
>> Konstantin
>
> Hi Konstantin,
>
> The benefit is to provide documented byte ordering for data types
> software is manipulating to determine when network to CPU (or CPU to
> network) conversion must be performed.
Why can we not just document the variables with doxygen as to the BE or LE expected type. Adding a new type is not going to solve the problem of someone using it incorrectly. In the function header doc it should state the expected type.
Adding yet another type is just going to confuse people as some drivers or code may never get changed. Also some drivers are common to many other systems, which means we would have to move the typedefs over to those systems like Linux and FreeBSD for the common sections.
>
> Regards,
>
> --
> Nélio Laranjeiro
> 6WIND
Regards,
Keith
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Adrien Mazarguil @ 2016-12-06 13:59 UTC (permalink / raw)
To: Ananyev, Konstantin
Cc: Thomas Monjalon, dev@dpdk.org, Rahul Lakkireddy, Stephen Hurd,
Jan Medala, Jakub Palider, John Daley, Alejandro Lucero,
Harish Patil, Rasesh Mody, Jerin Jacob, Yuanhan Liu, Yong Wang,
Kulasek, TomaszX, olivier.matz@6wind.com
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0E45D6@irsmsx105.ger.corp.intel.com>
Hi Konstantin,
On Tue, Dec 06, 2016 at 10:56:26AM +0000, Ananyev, Konstantin wrote:
>
> Hi Adrien,
>
> >
> > On Mon, Dec 05, 2016 at 04:43:52PM +0000, Ananyev, Konstantin wrote:
> > [...]
> > > > On Fri, Dec 02, 2016 at 01:00:55AM +0000, Ananyev, Konstantin wrote:
> > > > [...]
> > > > > > On Wed, Nov 30, 2016 at 10:54:50AM +0000, Ananyev, Konstantin wrote:
> > > > > > [...]
> > > > > > > Do you have anything particular in mind here?
> > > > > >
> > > > > > Nothing in particular, so for the sake of the argument, let's suppose that I
> > > > > > would like to add a field to expose some limitation that only applies to my
> > > > > > PMD during TX but looks generic enough to make sense, e.g. maximum packet
> > > > > > size when VLAN tagging is requested.
> > > > >
> > > > > Hmm, I didn't hear about such limitations so far, but if it is real case -
> > > > > sure, feel free to submit the patch.
> > > >
> > > > I won't, that was hypothetical.
> > >
> > > Then why we discussing it? :)
> >
> > Just to make a point, which is that new limitations may appear anytime and
> > tx_prepare() can now be used to check for them. First patch of the series
> > does it:
> >
> > + uint16_t nb_seg_max; /**< Max number of segments per whole packet. */
> > + uint16_t nb_mtu_seg_max; /**< Max number of segments per one MTU */
> >
> > And states that:
> >
> > + * For each packet to send, the rte_eth_tx_prepare() function performs
> > + * the following operations:
> > + *
> > + * - Check if packet meets devices requirements for tx offloads.
> > + *
> > + * - Check limitations about number of segments.
> > + *
> > + * - Check additional requirements when debug is enabled.
> > + *
> > + * - Update and/or reset required checksums when tx offload is set for packet.
>
> I think I already explained in my previous email why I think that
> nb_seg_max and nb_mtu_seg_max are not redundant because of tx_prepare().
> From my point they are complement to tx_prepare():
> Even if people do use tx_prepare() they still should take this information into account.
> As an example ixgbe can't TX packets with then 40 segments.
> tx_prepare() for ixgbe will flag that issue, but it can't make a decision on user behalf
> what to do in that case: drop the packet, try to coalesce it into the packet with less
> number of segments, split the packet into several smaller, etc.
> That's up to user to make such decision, and to make it, user might need this information.
Yet tx_prepare() has already the ability to update mbuf contents, issue is
what will this function do in the future, where will it stop? It is defined
in a way that each PMD does what it wants to make mbufs edible for
tx_burst(), because of this applications will just always call it to be on
the safe side.
> > It's like making this function mandatory IMO.
>
> That's probably where confusion starts: I don't think that
> tx_prepare() should be mandatory for the user to call.
> Yes, it should be a recommended way.
> But the user still should have the ability to by-pass it,
> if he believes there is no need for it, or he prefers to implement
> the same functionality on his own.
> As an example, if the user knows that he is going to send a group
> of one-segment packets that don't require any tx offloads, he can safely skip
> tx_prepare() for them.
I understand your point, and agree with the example you provide. Many
applications do not know what's inside mbufs though, except perhaps that
they contain TCP and may want to perform TSO because of that. Those will
have to call tx_prepare() to be future-proof.
> > > > > > PMDs are free to set that field to some
> > > > > > special value (say, 0) if they do not care.
> > > > > >
> > > > > > Since that field exists however, conscious applications should check its
> > > > > > value for each packet that needs to be transmitted. This extra code causes a
> > > > > > slowdown just by sitting in the data path. Since it is not the only field in
> > > > > > that structure, the performance impact can be significant.
>
> Conscious user will probably use this information at the stage of packet formation.
> He probably has to do this sort of things for large packets anyway:
> Check what is the underlying mtu, to decide does he need to split the packet,
> or enable tso for it, etc.
There are already too many things to check, applications probably won't mind
a little help from PMDs. If we keep adding fields to this structure, we'll
have to provide some sort of PMD-specific function that checks what is
relevant.
Furthermore, assuming most packets are fine and do not require any extra
processing, what is rejected by tx_burst() could enter some unlikely() path
that attempts to rectify and re-send them. That would at least optimize the
common scenario.
> > > > > > Even though this code is inside applications, it remains unfair to PMDs for
> > > > > > which these tests are irrelevant. This problem is identified and addressed
> > > > > > by tx_prepare().
> > > > >
> > > > > I suppose the question is why do we need:
> > > > > uint16_t nb_seg_max;
> > > > > uint16_t nb_mtu_seg_max;
> > > > > as we now have tx_prepare(), right?
> > > > >
> > > > > For two reasons:
> > > > > 1. Some people might feel that tx_prepare() is not good (smart/fast) enough
> > > > > for them and would prefer to do necessary preparations for TX offloads themselves.
> > > > >
> > > > > 2. Even if people do use tx_prepare() they still should take this information into accout.
> > > > > As an example ixgbe can't TX packets with then 40 segments.
> > > > > Obviously ixbge_tx_prep() performs that check and returns an error.
> > > >
> > > > Problem is that tx_prepare() also provides safeties which are not part of
> > > > tx_burst(), such as not going over nb_mtu_seg_max. Because of this and the
> > > > fact struct rte_eth_desc_lim can grow new fields anytime, application
> > > > developers will be tempted to just call tx_prepare() and focus on more
> > > > useful things.
> > >
> > > NP with that, that was an intention beyond introducing it.
> > >
> > > > Put another way, from a user's point of view, tx_prepare() is an opaque
> > > > function that greatly increases tx_burst()'s ability to send mbufs as
> > > > requested, with extra error checking on top; applications not written to run
> > > > on a specific PMD/device (all of them ideally) will thus call tx_prepare()
> > > > at some point.
> > > >
> > > > > But it wouldn't try to merge/reallocate mbufs for you.
> > > > > User still has to do it himself, or just prevent creating such long chains somehow.
> > > >
> > > > Yes, that's another debate. PMDs could still implement a software fallback
> > > > for unlikely slow events like these. The number of PMDs is not going to
> > > > decrease, each device having its own set of weird limitations in specific
> > > > cases, PMDs should do their best to process mbufs even if that means slowly
> > > > due to the lack of preparation.
> > > >
> > > > tx_prepare() has its uses but should really be optional, in the sense that
> > > > if that function is not called, tx_burst() should deal with it somehow.
> > >
> > > As I said before, I don't think it is a good idea to put everything in tx_burst().
> > > If PMD driver prefer things that way, yes tx_burst() can deal with each and
> > > possible offload requirement itself, but it shouldn't be mandatory.
> >
> > In effect, having to call tx_prepare() otherwise makes this step mandatory
> > anyway. Looks like we are not going to agree here.
> >
> > > > > > Thanks to tx_prepare(), these checks are moved back into PMDs where they
> > > > > > belong. PMDs that do not need them do not have to provide support for
> > > > > > tx_prepare() and do not suffer any performance impact as result;
> > > > > > applications only have to make sure tx_prepare() is always called at some
> > > > > > point before tx_burst().
> > > > > >
> > > > > > Once you reach this stage, you've effectively made tx_prepare() mandatory
> > > > > > before tx_burst(). If some bug occurs, then perhaps you forgot to call
> > > > > > tx_prepare(), you just need to add it. The total cost for doing TX is
> > > > > > therefore tx_prepare() + tx_burst().
> > > > > >
> > > > > > I'm perhaps a bit pessimistic mind you, but I do not think tx_prepare() will
> > > > > > remain optional for long. Sure, PMDs that do not implement it do not care,
> > > > > > I'm focusing on applications, for which the performance impact of calling
> > > > > > tx_prepare() followed by tx_burst() is higher than a single tx_burst()
> > > > > > performing all the necessary preparation at once.
> > > > > >
> > > > > > [...]
> > > > > > > > Following the same logic, why can't such a thing be made part of the TX
> > > > > > > > burst function as well (through a direct call to rte_phdr_cksum_fix()
> > > > > > > > whenever necessary). From an application standpoint, what are the advantages
> > > > > > > > of having to:
> > > > > > > >
> > > > > > > > if (tx_prep()) // iterate and update mbufs as needed
> > > > > > > > tx_burst(); // iterate and send
> > > > > > > >
> > > > > > > > Compared to:
> > > > > > > >
> > > > > > > > tx_burst(); // iterate, update as needed and send
> > > > > > >
> > > > > > > I think that was discussed extensively quite a lot previously here:
> > > > > > > As Thomas already replied - main motivation is to allow user
> > > > > > > to execute them on different stages of packet TX pipeline,
> > > > > > > and probably on different cores.
> > > > > > > I think that provides better flexibility to the user to when/where
> > > > > > > do these preparations and hopefully would lead to better performance.
> > > > > >
> > > > > > And I agree, I think this use case is valid but does not warrant such a high
> > > > > > penalty when your application does not need that much flexibility. Simple
> > > > > > (yet conscious) applications need the highest performance. Complex ones as
> > > > > > you described already suffer quite a bit from IPCs and won't mind a couple
> > > > > > of extra CPU cycles right?
> > > > >
> > > > > It would mean an extra cache-miss for every packet, so I think performance hit
> > > > > would be quite significant.
> > > >
> > > > A performance hit has to occur somewhere regardless, because something has
> > > > to be done in order to send packets that need it. Whether this cost is in
> > > > application code or in a PMD function, it remains part of TX.
> > >
> > > Depending on the place the final cost would differ quite a lot.
> > > If you call tx_prepare() somewhere close to the place where you fill the packet header
> > > contents, then most likely the data that tx_prepare() has to access will be already in the cache.
> > > So the performance penalty will be minimal.
> > > If you'll try to access the same data later (at tx_burst), then the possibility that it would still
> > > be in cache is much less.
> > > If you calling tx_burst() from other core then data would for sure be out of cache,
> > > and even worse can still be in another core cache.
> >
> > Well sure, that's why I also think tx_prepare() has its uses, only that
> > since tx_prepare() is optional, tx_burst() should provide the same
> > functionality when tx_prepare() is not called.
>
> As I understand, to implement what you are proposing (TX_PREPARED mbuf->ol_flag)
> it will be required:
>
> a) Modify all existing applications that do similar to tx_prepare() stuff on their own,
> otherwise they'll would hit performance penalty.
> b) Modify at least all Intel PMDs and might be some others too (vmxnet3?).
>
> Step b) probably wouldn't cause any significant performance impact straightway,
> but it's for sure wouldn't make things faster, and would increase tx_burst() code
> complexity quite a lot.
> From other side, I can't see any real benefit that we will have in return.
> So I still opposed to that idea.
Applications gain the ability to perform tx_burst() with offloads without
having to prepare anything. Currently these applications either cannot use
offloads at all or need to perform PMD-specific voodoo first. The generic
alternative to this scenario being tx_prepare(), PMDs have to make this step
as cheap as possible.
Yes that would slow down existing applications, people may find it
acceptable since we're modifying the TX API here.
> > > > > About the 'simple' case when tx_prep() and tx_burst() are called on the same core,
> > > > > Why do you believe that:
> > > > > tx_prep(); tx_burst(); would be much slower than tx_burst() {tx_prep(), ...}?
> > > >
> > > > I mean instead of two function calls with their own loops:
> > > >
> > > > tx_prepare() { foreach (pkt) { check(); extra_check(); ... } }
> > > >
> > > > tx_burst() { foreach (pkt) { check(); stuff(); ... } }
> > > >
> > > > You end up with one:
> > > >
> > > > tx_burst() { foreach (pkt) { check(); extra_check(); stuff(); ... } }
> > > >
> > > > Which usually is more efficient.
> > >
> > > I really doubt that.
> > > If it would be that, what is the point to process packet in bulks?
> > > Usually dividing processing into different stages and at each stage processing
> > > multiple packet at once helps to improve performance.
> > > At least for IA.
> > > Look for example how we had to change l3fwd to improve its performance.
> >
> > Depends quite a bit on usage pattern. It is less efficient for applications
> > that do not modify mbuf contents because of the additional function call and
> > inner loop.
>
> If the application doesn't modify mbuf contents that it can simply skip calling tx_prepare().
What if that same application wants to enable some offload as well?
> > Note that I'm only pushing for the ability to conveniently address both
> > cases with maximum performance.
> >
> > > > > tx_prep() itself is quite expensive, let say for Intel HW it includes:
> > > > > - read mbuf fileds (2 cache-lines),
> > > > > - read packet header (1/2 cache-lines)
> > > > > - calculate pseudo-header csum
> > > > > - update packet header
> > > > > Comparing to that price of extra function call seems neglectable
> > > > > (if we TX packets in bursts of course).
> > > >
> > > > We agree its performance is a critical issue then, sharing half the read
> > > > steps with tx_burst() would make sense to me.
> > >
> > > I didn't understand that sentence.
> >
> > I meant this step can be shared (in addition to loop etc):
> >
> > - read mbuf fileds (2 cache-lines),
>
> Ah ok, you still believe that mixing tx_burst and tx_prepare code together
> would give us noticeable performance benefit.
> As I said above, I don't think it would, but you are welcome to try and
> prove me wrong.
Depends on what you call noticeable. I guess we can at least agree having
two separate functions and loops cause more instructions to be generated and
executed.
Now for the number of spent CPU cycles, depends of course whether mbufs are
still hot into the cache or not, and as I told you in my opinion we'll
usually see applications calling tx_prepare() just before tx_burst() to
benefit from offloads.
> > > > > > Yes they will, therefore we need a method that satisfies both cases.
> > > > > >
> > > > > > As a possible solution, a special mbuf flag could be added to each mbuf
> > > > > > having gone through tx_prepare(). That way, tx_burst() could skip some
> > > > > > checks and things it would otherwise have done.
> > > > >
> > > > > That's an interesting idea, but it has one drawback:
> > > > > As I understand, it means that from now on if user doing preparations on his own,
> > > > > he had to setup this flag, otherwise tx_burst() would do extra unnecessary work.
> > > > > So any existing applications that using TX offloads and do preparation by themselves
> > > > > would have to be modified to avoid performance loss.
> > > >
> > > > In my opinion, users should not do preparation on their own.
> > >
> > > People already do it now.
> >
> > But we do not want them to anymore thanks to this new API, for reasons
> > described in the motivation section of the cover letter, right?
>
> We probably wouldn't recommend that, but if people would like to use their own stuff,
> or shortcuts - I don't want to stop them here.
>
> >
> > > > If we provide a
> > > > generic method, it has to be fast enough to replace theirs. Perhaps not as
> > > > fast since it would work with all PMDs (usual trade-off), but acceptably so.
> > > >
> > > > > > Another possibility, telling the PMD first that you always intend to use
> > > > > > tx_prepare() and getting a simpler/faster tx_burst() callback as a result.
> > > > >
> > > > > That what we have right now (at least for Intel HW):
> > > > > it is a user responsibility to do the necessary preparations/checks before calling tx_burst().
> > > > > With tx_prepare() we just remove from user the headache to implement tx_prepare() on his own.
> > > > > Now he can use a 'proper' PMD provided function.
> > > > >
> > > > > My vote still would be for that model.
> > > >
> > > > OK, then in a nutshell:
> > > >
> > > > 1. Users are not expected to perform preparation/checks themselves anymore,
> > > > if they do, it's their problem.
> > >
> > > I think we need to be backward compatible here.
> > > If the existing app doing what tx_prepare() supposed to do, it should keep working.
> >
> > It should work, only if they keep doing it as well as call tx_burst()
> > directly, they will likely get lower performance.
> >
> > > > 2. If configured through an API to be defined, tx_burst() can be split in
> > > > two and applications must call tx_prepare() at some point before
> > > > tx_burst().
> > > >
> > > > 3. Otherwise tx_burst() should perform the necessary preparation and checks
> > > > on its own by default (when tx_prepare() is not expected).
> > >
> > > As I said before, I don't think it should be mandatory for tx_burst() to do what tx_prepare() does.
> > > If some particular implementation of tx_burst() prefers to do things that way - that's fine.
> > > But it shouldn't be required to.
> >
> > You're right, however applications might find it convenient. I think most
> > will end up with something like the following:
> >
> > if (tx_prepare(pkts))
> > tx_burst(pkts));
>
> Looking at existing DPDK apps - most of them do use some sort of TX bufferization.
> So, even in a simplistic app it would probably be:
>
> tx_prepare(pkts);
> tx_buffer(pkts);
We're down to my word against yours here I guess, to leave the choice to
application developers, we'd need to provide tx_prepare() and a simpler
tx_burst() as well as the ability to call tx_burst() directly and still get
offloads.
> > > > 4. We probably still need some mbuf flag to mark mbufs that cannot be
> > > > modified, the refcount could also serve as a hint.
> > >
> > > If mbuf can't be modified, you probably just wouldn't call the function that supposed to do that,
> > > tx_prepare() in that case.
> >
> > I think it would be easier to document what offload flags may cause the
> > tx_burst() function to modify mbuf contents, so applications have the
> > ability to set or strip these flags on a mbuf basis. That way there is no
> > need to call tx_prepare() without knowing exactly what it's going to do.
>
> Not sure I understand what exactly do you propose in the last paragraph?
That for each TX offload flag, we document whether preparation might cause a
mbuf to be written to during the tx_prepare()/tx_burst() phase. One of the
reasons for tx_prepare() being:
4) Fields in packet may require different initialization (like e.g. will
require pseudo-header checksum precalculation, sometimes in a
different way depending on packet type, and so on). Now application
needs to care about it.
If we determine what offloads may cause mbuf contents to change (all of them
perhaps?), then applications can easily strip those flags from outgoing
"const" mbufs. Then it becomes acceptable for tx_burst() to modify mbuf
contents as per user request, which removes one reason to rely on
tx_prepare() for these.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH 01/32] doc: add dpaa2 nic details
From: Hemant Agrawal @ 2016-12-06 13:58 UTC (permalink / raw)
To: Mcnamara, John, dev@dpdk.org
Cc: thomas.monjalon@6wind.com, Richardson, Bruce,
shreyansh.jain@nxp.com
In-Reply-To: <B27915DBBA3421428155699D51E4CFE20266ECED@IRSMSX103.ger.corp.intel.com>
On 12/5/2016 10:42 PM, Mcnamara, John wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hemant Agrawal
>> Sent: Sunday, December 4, 2016 6:17 PM
>> To: dev@dpdk.org
>> Cc: thomas.monjalon@6wind.com; Richardson, Bruce
>> <bruce.richardson@intel.com>; shreyansh.jain@nxp.com; Hemant Agrawal
>> <hemant.agrawal@nxp.com>
>> Subject: [dpdk-dev] [PATCH 01/32] doc: add dpaa2 nic details
>>
>> Add the dpaa2 architecture and pmd details
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>
>
> Thanks for the detailed doc.
>
> There is a whitespace warning on merge. Also, some comments below.
Thanks for all your comments. I will take care of these in v2
>> +
>> +DPAA2 Poll Mode Driver
>> +===============================
>> +
>
> The underline should match the length of the title. Also, there should be
> 1 newline after each section header. This is in this example but there are
> several without it.
>
>
>
>> +
>> +NXP DPAA2 (Data Path Acceleration Architecture Gen2)
>> +----------------------------------------------------
>> +
>> +This section provides an overview of the NXP DPAA2 architecture and how
>> +it is integrated into the DPDK.
>
> The text says "This section" which implies that the following sections are
> subsections. In that case the underline for the sub sections should probably
> be: ~~~~~~~~~~~~~~~~
>
> http://dpdk.org/doc/guides/contributing/documentation.html#section-headers
>
> This applies to some other section introductions below as well.
>
>> +
>> +Contents summary
>> +- DPAA2 overview
>> +- Overview of DPAA2 objects
>> +- DPAA2 driver architecture overview
>
> This needs a newline after "Contents summary" to generate a list.
>
>
>> +
>> +DPAA2 Overview
>> +--------------
>> +Refer: `FSL MC BUS in Linux Kernel
>> <https://www.kernel.org/doc/readme/drivers-staging-fsl-mc-README.txt>`_.
>
> s/Refer/Reference/
>
> Note, I was very impressed with this doc until I saw that the text was all
> in the other README file. :-)
>
>
>> +DPAA2 is a hardware architecture designed for high-speeed network
>
> s/speeed/speed/
>
>
>
>> + Toggle display of generic debugging messages
>> +
>> +- ``CONFIG_RTE_LIBRTE_DPAA2_USE_PHYS_IOVA`` (default ``y``)
>> +
>> + Toggle to use physical address vs virtual address for hardware
>> acceleraters.
>
> s/acceleraters/accelerators/
>
>
>
>> +
>> +Driver Compilation
>> +~~~~~~~~~~~~~~~~~~
>> +
>> +To compile the DPAA2 PMD for Linux arm64 gcc target, run the following
>> +“make†command:
>
> Non-ascii/non-Unicode characters here. Use ``make`` instead.
>
>
>> +
>> +# configure the resource container
>> +
>> + configure resources in MC and create the DPRC container
>> + export the DPRC container
>> + e.g. export DPRCT=dprc.2
>
> Should be #. and have a console block or directive like the example below.
>
>
>
>> +
>> +#. Start ``testpmd`` with basic parameters:
>> +
>> + .. code-block:: console
>> +
>> + ./arm64-dpaa2-linuxapp-gcc/testpmd -c 0xff -n 1 \
>> + -- -i --portmask=0x1 --nb-cores=1 --no-flush-rx
>> +
>> +
>
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox