Re: [ovs-dev] [PATCH v1 4/4] travis: Enable OVS Travis CI for arm

2019-11-24 Thread Lance Yang (Arm Technology China)



> -Original Message-
> From: Ilya Maximets 
> Sent: Friday, November 22, 2019 6:36 PM
> To: Lance Yang (Arm Technology China) ; Ilya Maximets
> ; ovs-dev@openvswitch.org
> Cc: Jieqiang Wang (Arm Technology China) ; Gavin Hu 
> (Arm
> Technology China) ; Jingzhao Ni (Arm Technology China)
> ; dwil...@us.ibm.com; nd ; Ruifeng Wang 
> (Arm
> Technology China) ; Yanqin Wei (Arm Technology China)
> 
> Subject: Re: [ovs-dev] [PATCH v1 4/4] travis: Enable OVS Travis CI for arm
>
> On 22.11.2019 10:35, Lance Yang (Arm Technology China) wrote:
> > Hi Ilya,
> >
> > Thanks for your reply. Please see the inline reply.
> >
> > Best regards,
> > Lance Yang
> >
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Friday, November 22, 2019 3:47 AM
> >> To: Lance Yang (Arm Technology China) ; ovs-
> >> d...@openvswitch.org
> >> Cc: Jieqiang Wang (Arm Technology China) ;
> >> Gavin Hu (Arm Technology China) ; Jingzhao Ni (Arm
> >> Technology China) ; dwil...@us.ibm.com; nd
> >> ; Ruifeng Wang (Arm Technology China)
> >> 
> >> Subject: Re: [ovs-dev] [PATCH v1 4/4] travis: Enable OVS Travis CI
> >> for arm
> >>
> >> On 20.11.2019 9:15, Lance Yang wrote:
> >>> Enable part of travis jobs with gcc compiler for arm64 architecture
> >>>
> >>> 1. Add arm jobs into the matrix in .travis.yml and define dpdk cache
> >>> directory for arm64 architecture.
> >>> 2. Temporarily disable sparse checker because of static code
> >>> checking failure on arm64
> >>
> >> Could you provide the sparse error messages?
> >> Maybe we could fix them instead of disabling.
> >>
> >>>
> >>> Successful travis build jobs report:
> >>> https://travis-ci.org/yzyuestc/ovs/builds/614299933
> >>
> >> There are some issues in Travis in above build.  It seems that Travis
> >> fails to upload the resulted cache directory due to missing md5deep:
> >>
> >> ""
> > [Lance]
> > The issue occurred because sparse checker does not recognize some arm neon 
> > intrinsics:
> >
> > 1. Some components in Open vSwitch source code directly includes the header 
> > file
> .
> >
> > 2. When compiling OvS with DPDK, OvS includes some DPDK headers, which
> > sometimes includes . You can see the details of the error
> > in the report at: https://travis-ci.org/yzyuestc/ovs/jobs/615414391
> > after line 2693
> >
> > For the first issue where OvS directly imported the arm_neon.h,  I can fix 
> > it in my patch set
> v2. However, for the second issue that DPDK project has imported the arm neon 
> intrinsics, it
> seems that I am unable to fix this issue until DPDK solved it.
>
> Hi.
>
> Thanks for the information.
> Could, you, please, check if the following patch fixes the issue:
> https://patchwork.ozlabs.org/patch/1199398/
> ?
[Lance]
Thanks for your reply.

I downloaded and applied the patch from patchwork. However, it seems that the 
patch does not work as expected. I attached the original travis report link: 
https://travis-ci.org/yzyuestc/ovs/jobs/616442657#L2692. You can check the 
error after line 2692.

Thanks for your time.
>
> >
> >> store build cache
> >> 0.02s5.41sInstalling md5deep
> >> Reading package lists...
> >> Building dependency tree...
> >> Reading state information...
> >> md5deep is already the newest version (4.4-2).
> >> 0 upgraded, 0 newly installed, 0 to remove and 8 not upgraded.
> >> /home/travis/.casher/bin/casher: line 230: md5deep: command not found
> >> nothing changed ""
> >>
> > [Lance]
> > Thanks for pointing out the issue. The upload cache on arm is not
> > successful, that means arm jobs will always clone and build DPDK. I
> > just posted this issue to Travis CI community. You can see:
> > https://travis-ci.community/t/no-cache-support-on-arm64/5416/21
>
> Thanks!  Let's wait for their reply.
>
> Best regards, Ilya Maximets.
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.

2019-11-24 Thread David Marchand
On Fri, Nov 22, 2019 at 3:04 PM Ilya Maximets  wrote:
> >>> Rather than exclude a set of flags, I would touch/copy only the flags
> >>> that OVS uses/understands.
> >>>
> >>> When OVS uses a DPDK flag, it has an associated API and meaning wrt
> >>> the rte_mbuf and the data.
> >>> - when the flags are reset in OVS, that's because something has been
> >>> done to the data (and the checksums and the rss hash must be
> >>> reevaluated),
> >>> - when a buffer is copied, OVS copies the data and the context that
> >>> matters to OVS,
> >>>
> >>> Anything else should be discarded when copying to a new dp-packet.
> >>
> >> Yes, that was the first version I wanted to implement, i.e. to collect
> >> all the flags that OVS really uses and clear/copy only these flags.
> >>
> >> But then I started to think about 'ring' ports and that we might send
> >> mbufs with incorrect flags set after the packet modification to the
> >> external application.  This doesn't sound good.  Because if OVS doesn't
> >> use them doesn't mean that other applications doesn't.  So, I've end up
> >> with the current logic with clearing everything except the memory layout
> >> flags.
> >>
> >> Does it make sense?  What do you think?
> >
> > Before sending a packet through a dpdk ring, OVS will touch the data
> > and update the relevant flags as far as it is concerned.
> >
> > The application can read/touch those mbuf flags as long as it complies
> > with the DPDK APIs, this concerns both the flags that OVS is aware of,
> > and the rest.
> > So when getting this mbuf back, the flags should be valid.
> >
> > After this, OVS can do what it wants on the packet, it will only touch
> > the flags it understands.
> >
> > What am I missing?
>
> I agree that OVS itself will work OK by only considering flags it
> really uses.  I'm concerned about the second application that receives
> mbuf from the OVS via ring port.  For example, I see that ixgbe driver
> almost unconditionally parses vlans and sets PKT_RX_VLAN along with
> mbuf->vlan_tci.  If OVS will not clear this flag while sending to

- Well, as I said, if OVS touches the data (pops a vlan), it must
update the flags (PKT_RX_VLAN).
This is overkill if the mbuf does not leave OVS, so it could be
cleared unconditionally before jumping in the ring.


- What kind of applications can read those rings?
Are those dpdk secondary applications?
Asking, since we know that the multiprocess stuff is not really that
robust and/or usable with OVS.


-- 
David Marchand

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH 15/20] netdev-offload-dpdk-flow: Support offload of output action

2019-11-24 Thread Eli Britstein


On 11/22/2019 6:19 PM, Ilya Maximets wrote:
> On 20.11.2019 16:28, Eli Britstein wrote:
>> Signed-off-by: Eli Britstein 
>> Reviewed-by: Oz Shlomo 
>> ---
>>   NEWS   |  2 +
>>   lib/netdev-offload-dpdk-flow.c | 87 
>> --
>>   2 files changed, 85 insertions(+), 4 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 88b818948..ca9c2b230 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -10,6 +10,8 @@ Post-v2.12.0
>>  if supported by libbpf.
>>* Add option to enable, disable and query TCP sequence checking in
>>  conntrack.
>> +   - DPDK:
>> + * Add hardware offload support for output actions.
>>   
>>   v2.12.0 - 03 Sep 2019
>>   -
>> diff --git a/lib/netdev-offload-dpdk-flow.c b/lib/netdev-offload-dpdk-flow.c
>> index dbbc45e99..6e7efb315 100644
>> --- a/lib/netdev-offload-dpdk-flow.c
>> +++ b/lib/netdev-offload-dpdk-flow.c
>> @@ -274,6 +274,28 @@ ds_put_flow_action(struct ds *s, const struct 
>> rte_flow_action *actions)
>>   } else {
>>   ds_put_cstr(s, "  RSS = null\n");
>>   }
>> +} else if (actions->type == RTE_FLOW_ACTION_TYPE_COUNT) {
>> +const struct rte_flow_action_count *count = actions->conf;
>> +
>> +ds_put_cstr(s, "rte flow count action:\n");
>> +if (count) {
>> +ds_put_format(s,
>> +  "  Count: shared=%d, id=%d\n",
>> +  count->shared, count->id);
>> +} else {
>> +ds_put_cstr(s, "  Count = null\n");
>> +}
>> +} else if (actions->type == RTE_FLOW_ACTION_TYPE_PORT_ID) {
>> +const struct rte_flow_action_port_id *port_id = actions->conf;
>> +
>> +ds_put_cstr(s, "rte flow port-id action:\n");
>> +if (port_id) {
>> +ds_put_format(s,
>> +  "  Port-id: original=%d, id=%d\n",
>> +  port_id->original, port_id->id);
>> +} else {
>> +ds_put_cstr(s, "  Port-id = null\n");
>> +}
>>   } else {
>>   ds_put_format(s, "unknown rte flow action (%d)\n", actions->type);
>>   }
>> @@ -531,19 +553,76 @@ netdev_dpdk_flow_patterns_add(struct flow_patterns 
>> *patterns,
>>   return 0;
>>   }
>>   
>> +static void
>> +netdev_dpdk_flow_add_count_action(struct flow_actions *actions)
>> +{
>> +struct rte_flow_action_count *count = xzalloc(sizeof *count);
>> +
>> +count->shared = 0;
>> +/* Each flow has a single count action, so no need of id */
>> +count->id = 0;
>> +add_flow_action(actions, RTE_FLOW_ACTION_TYPE_COUNT, count);
>> +}
>> +
>> +static void
>> +netdev_dpdk_flow_add_port_id_action(struct flow_actions *actions,
>> +struct netdev *outdev)
>> +{
>> +struct rte_flow_action_port_id *port_id = xzalloc(sizeof *port_id);
>> +
>> +port_id->id = netdev_dpdk_get_port_id(outdev);
>> +add_flow_action(actions, RTE_FLOW_ACTION_TYPE_PORT_ID, port_id);
>> +}
>> +
>> +static int
>> +netdev_dpdk_flow_add_output_action(struct flow_actions *actions,
>> +   const struct nlattr *nla,
>> +   struct offload_info *info)
>> +{
>> +struct netdev *outdev;
>> +odp_port_t port;
>> +
>> +port = nl_attr_get_odp_port(nla);
>> +outdev = netdev_ports_get(port, info->dpif_class);
>> +if (outdev == NULL) {
>> +VLOG_DBG_RL(&error_rl,
>> +"Cannot find netdev for odp port %d", port);
>> +return -1;
>> +}
>> +if (!netdev_dpdk_flow_api_supported(outdev)) {
> This doesn't look correct.  I mean, maybe we need to check if port is
> really the port on a same piece of hardware.  Will the flow setup fail
> in this case?  Will code fallback to the MARK+RSS?

We cannot check if the port is on the same HW, and it is also wrong from 
the application point of view. If the operation cannot be supported, it 
is in the driver (DPDK) scope to fail it.

You are right about the fallback. I'll fix it in v2.

>
>> +VLOG_DBG_RL(&error_rl,
>> +"Output to %s cannot be offloaded",
>> +netdev_get_name(outdev));
>> +return -1;
>> +}
>> +
>> +netdev_dpdk_flow_add_count_action(actions);
>> +netdev_dpdk_flow_add_port_id_action(actions, outdev);
>> +netdev_close(outdev);
>> +
>> +return 0;
>> +}
>> +
>>   int
>>   netdev_dpdk_flow_actions_add(struct flow_actions *actions,
>>struct nlattr *nl_actions,
>>size_t nl_actions_len,
>> - struct offload_info *info OVS_UNUSED)
>> + struct offload_info *info)
>>   {
>>   struct nlattr *nla;
>>   size_t left;
>>   
>>   NL_ATTR_FOR_EACH_UNSAFE (nla, left, nl_actions, nl_actions_len) {
>> -VLOG_DBG_RL(&error_rl,
>> -"Unsupported action type 

Re: [ovs-dev] [PATCH 07/20] netdev-offload-dpdk: Introduce flow flush callback

2019-11-24 Thread Eli Britstein


On 11/22/2019 6:06 PM, Ilya Maximets wrote:
> On 20.11.2019 16:28, Eli Britstein wrote:
>> Introduce flow flush callback for dpdk offloaded flows.
>>
>> Signed-off-by: Eli Britstein 
>> Reviewed-by: Oz Shlomo 
>> ---
>>   lib/netdev-offload-dpdk.c | 16 
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>> index 6e1ca8a0d..64873759d 100644
>> --- a/lib/netdev-offload-dpdk.c
>> +++ b/lib/netdev-offload-dpdk.c
>> @@ -307,6 +307,21 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev, 
>> const ovs_u128 *ufid,
>>   return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow);
>>   }
>>   
>> +static int
>> +netdev_offload_dpdk_flow_flush(struct netdev *netdev)
>> +{
>> +struct rte_flow_error error;
>> +int ret;
>> +
>> +ret = netdev_dpdk_rte_flow_flush(netdev, &error);
> I don't think that it's enough to only flush flows from the device,
> you need to destroy all the structures allocated for already offloaded
> flows in OVS, i.e. at least call ufid_to_rte_flow_disassociate()
> for each of them.

Well, I admit I haven't followed all the code of it. I took 
netdev-offload-tc.c flush and adapted. It also didn't remove mappings. 
Anyway, if you think the TC code is also bad, I'll drop those 2 commits 
from this series for now

>
>> +if (ret) {
>> +VLOG_ERR("%s: rte flow flush error: %u : message : %s\n",
>> + netdev_get_name(netdev), error.type, error.message);
>> +}
>> +
>> +return ret;
>> +}
>> +
>>   static int
>>   netdev_offload_dpdk_init_flow_api(struct netdev *netdev)
>>   {
>> @@ -315,6 +330,7 @@ netdev_offload_dpdk_init_flow_api(struct netdev *netdev)
>>   
>>   const struct netdev_flow_api netdev_offload_dpdk = {
>>   .type = "dpdk_flow_api",
>> +.flow_flush = netdev_offload_dpdk_flow_flush,
>>   .flow_put = netdev_offload_dpdk_flow_put,
>>   .flow_del = netdev_offload_dpdk_flow_del,
>>   .init_flow_api = netdev_offload_dpdk_init_flow_api,
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH 12/20] dpif-netdev: Read hw stats during flow_dump_next() call

2019-11-24 Thread Eli Britstein


On 11/22/2019 6:10 PM, Ilya Maximets wrote:
> On 20.11.2019 16:28, Eli Britstein wrote:
>> From: Ophir Munk 
>>
>> Flow stats are retrieved by revalidator threads. Specifically for
>> dpif-netdev the function dpif_netdev_flow_dump_next() is called.
>> When the flow is fully offloaded reading the PMD SW stats alone will
>> result in no updates and will cause the revalidator to falsely delete
>> the flow after some aging time.
>> This commit adds a new function called dp_netdev_offload_used() which
>> reads the hw counters during the flow_dump_next() call.
>> The new function calls netdev_flow_stats_get() which in turn reads the
>> hardware stats by calling DPDK rte_flow_query() API.
>>
>> Signed-off-by: Ophir Munk 
>> Reviewed-by: Oz Shlomo 
>> Signed-off-by: Eli Britstein 
>> ---
>>   lib/dpif-netdev.c | 40 +++-
>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 4720ba1ab..d9f28cfcd 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -527,6 +527,7 @@ struct dp_netdev_flow {
>>   
>>   bool dead;
>>   uint32_t mark;   /* Unique flow mark assigned to a flow */
>> +bool actions_offloaded;  /* true if flow is fully offloaded */
>>   
>>   /* Statistics. */
>>   struct dp_netdev_flow_stats stats;
>> @@ -2422,6 +2423,7 @@ dp_netdev_flow_offload_put(struct dp_flow_offload_item 
>> *offload)
>> offload->actions_len, &flow->mega_ufid, &info,
>> NULL);
>>   ovs_mutex_unlock(&pmd->dp->port_mutex);
>> +flow->actions_offloaded = info.actions_offloaded;
>>   
>>   if (ret) {
>>   goto err_free;
>> @@ -3073,7 +3075,7 @@ dp_netdev_flow_to_dpif_flow(const struct 
>> dp_netdev_flow *netdev_flow,
>>   flow->pmd_id = netdev_flow->pmd_id;
>>   get_dpif_flow_stats(netdev_flow, &flow->stats);
>>   
>> -flow->attrs.offloaded = false;
>> +flow->attrs.offloaded = netdev_flow->actions_offloaded;
>>   flow->attrs.dp_layer = "ovs";
> We, probably, should change the 'dp_layer' for fully offloaded flows.
>
>>   }
>>   
>> @@ -3244,6 +3246,7 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
>>   flow->dead = false;
>>   flow->batch = NULL;
>>   flow->mark = INVALID_FLOW_MARK;
>> +flow->actions_offloaded = false;
>>   *CONST_CAST(unsigned *, &flow->pmd_id) = pmd->core_id;
>>   *CONST_CAST(struct flow *, &flow->flow) = match->flow;
>>   *CONST_CAST(ovs_u128 *, &flow->ufid) = *ufid;
>> @@ -3598,6 +3601,37 @@ dpif_netdev_flow_dump_thread_destroy(struct 
>> dpif_flow_dump_thread *thread_)
>>   free(thread);
>>   }
>>   
>> +static int
>> +dpif_netdev_offload_used(struct dp_netdev_flow *netdev_flow,
>> + struct dp_netdev_pmd_thread *pmd)
>> +{
>> +int ret;
>> +struct dp_netdev_port *port;
>> +struct dpif_flow_stats stats;
>> +
>> +odp_port_t in_port = netdev_flow->flow.in_port.odp_port;
>> +
>> +ovs_mutex_lock(&pmd->dp->port_mutex);
>> +port = dp_netdev_lookup_port(pmd->dp, in_port);
> Use netdev_ports_get() instead.
> See: 
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1199525%2F&data=02%7C01%7Celibr%40mellanox.com%7Ce6156f21dee847deeff408d76f669285%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637100358654694310&sdata=HZzqYAJrtR91jAPyi1CmFK5VHy3qcuBMjXEVSfmD%2FT4%3D&reserved=0
OK
>
>> +if (!port) {
>> +ovs_mutex_unlock(&pmd->dp->port_mutex);
>> +return -1;
>> +}
>> +/* get offloaded stats */
>> +ret = netdev_flow_stats_get(port->netdev, &netdev_flow->mega_ufid, 
>> &stats);
>> +ovs_mutex_unlock(&pmd->dp->port_mutex);
>> +if (ret) {
>> +return -1;
>> +}
>> +if (stats.n_packets) {
>> +atomic_store_relaxed(&netdev_flow->stats.used, pmd->ctx.now / 1000);
>> +non_atomic_ullong_add(&netdev_flow->stats.packet_count, 
>> stats.n_packets);
>> +non_atomic_ullong_add(&netdev_flow->stats.byte_count, 
>> stats.n_bytes);
>> +}
>> +
>> +return 0;
>> +}
>> +
>>   static int
>>   dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_,
>>  struct dpif_flow *flows, int max_flows)
>> @@ -3638,6 +3672,10 @@ dpif_netdev_flow_dump_next(struct 
>> dpif_flow_dump_thread *thread_,
>>   netdev_flows[n_flows] = CONTAINER_OF(node,
>>struct dp_netdev_flow,
>>node);
>> +/* Read hardware stats in case of hardware offload */
>> +if (netdev_flows[n_flows]->actions_offloaded) {
>> +dpif_netdev_offload_used(netdev_flows[n_flows], pmd);
>> +}
>>   }
>>   /* When finishing dumping the current pmd thread, moves to
>>* the next. */
>>
___

Re: [ovs-dev] [PATCH 00/20] netdev datapath actions offload

2019-11-24 Thread Eli Britstein


On 11/22/2019 6:34 PM, Ilya Maximets wrote:
> On 20.11.2019 16:28, Eli Britstein wrote:
>> Currently, netdev datapath offload only accelerates the flow match
>> sequence by associating a mark per flow. This series introduces the full
>> offload of netdev datapath flows by having the HW also perform the flow
>> actions.
>>
>> This series adds HW offload for output, drop, set MAC, set IPv4, set
>> TCP/UDP ports and tunnel push actions using the DPDK rte_flow API.
>>
>> Travis passed: 
>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F614511472&data=02%7C01%7Celibr%40mellanox.com%7C893d54b1da3748fab12408d76f69dbcf%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637100372770275434&sdata=031VCVHeS7HxQB%2F6PvZHZOIWNGMl1BcZqMM0JbDKaa8%3D&reserved=0
>>
>> Eli Britstein (18):
>>sparse: rte_flow.h: Adapt according to DPDK 18.11.2
>>netdev-offload-dpdk: Refactor flow patterns and actions code
>>netdev-offload-dpdk-flow: Dynamically allocate pattern items
>>netdev-offload-dpdk: Fix typo
>>netdev-dpdk: Improve HW offload flow debuggability
>>netdev-dpdk: Introduce flow_flush method
>>netdev-offload-dpdk: Introduce flow flush callback
>>netdev-offload-dpdk: Framework for actions offload
>>netdev-dpdk: Introduce rte flow query function
>>netdev-offload-dpdk: Implement flow stats get
>>dpif-netdev: Populate dpif class field in offload struct
>>netdev-dpdk: Getter function for dpdk port id API
>>netdev-offload-dpdk-flow: Support offload of output action
>>netdev-offload-dpdk-flow: Support offload of drop action
>>netdev-offload-dpdk-flow: Support offload of set MAC actions
>>netdev-offload-dpdk-flow: Support offload of set IPv4 actions
>>netdev-offload-dpdk-flow: Support offload of clone tnl_push/output
>>  actions
>>netdev-offload-dpdk-flow: Support offload of set TCP/UDP ports actions
>>
>> Ophir Munk (2):
>>netdev: Add netdev function: flow_stats_get()
>>dpif-netdev: Read hw stats during flow_dump_next() call
>>
>>   NEWS  |   3 +
>>   include/sparse/rte_flow.h | 810 ++---
>>   lib/automake.mk   |   4 +-
>>   lib/dpif-netdev.c |  42 +-
>>   lib/netdev-dpdk.c |  76 
>>   lib/netdev-dpdk.h |  11 +
>>   lib/netdev-offload-dpdk-flow.c| 916 
>> ++
>>   lib/netdev-offload-dpdk-private.h |  65 +++
>>   lib/netdev-offload-dpdk.c | 535 +++---
>>   lib/netdev-offload-provider.h |   7 +
>>   lib/netdev-offload.c  |  12 +
>>   lib/netdev-offload.h  |   3 +
>>   12 files changed, 1969 insertions(+), 515 deletions(-)
>>   create mode 100644 lib/netdev-offload-dpdk-flow.c
>>   create mode 100644 lib/netdev-offload-dpdk-private.h
>>
> Hi. Thanks for working on this!
>
> I really roughly read the patch-set and made a few comments for
> the places that caught my eyes.
>
> Few general comments for the patches:
>
> * I don't understand why all of this refactoring was done. Maybe it's because
>I didn't try to apply the series and look at it as a complete change, but
>for now it's unclear for me, especially because we already refactored this
>code few times before and you're reverting some of these changes.
>Also the need to have netdev-offload-dpdk-flow.c is not clear.

I wanted to make it more ordered, and split to a new file as we plan to 
submit more patches in this area for VXLAN offload and later CT. Those 
will include a lot of code additions and maybe even more files. So, we 
can either keep it so a single file won't grow to be very large, or we 
can postpone it for later if it really grows like this. I prefer to keep 
it as is, unless you object.

What have I reverted?

>
> * Commit messages are needed.  Please, add some with a brief explanation
>on what happens and why.
I'll elaborate a bit more, though there are some commits (last ones) 
that there is really nothing to elaborate more than the subject.
>
> * If it's not hard, since you will re-spin the series anyway, please, add
>some periods to the ends of a subject lines.
Actually, I've seen that there is inconsistency of this style in OVS. 
Some commits have this period and some don't. For example in kernel, 
iproute2, dpdk gits, they are consistent *without* it. I think it should 
be consistent. I don't see the added value of it, and I think maybe we 
should adopt the dot-less style. What do you think?
>
> * You need to also update the 'Flow Hardware Offload' section in the
>Documentation/howto/dpdk.rst.
OK. In v2
>
> * And, please, mark these new features as experimental in NEWS.
OK. In v2
>
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev