Re: [ovs-dev] [patch] datapath-windows: Append tunnel info to upcall for correct template

[Amber Hu via dev]

Hi Alin,

Will update the patch and change it according to the comments.
Thanks very much.

BR/Amber

On 1/22/20, 12:43, "Alin Serdean"  wrote:

Hi Amber,

The patch fails to apply:
"error: corrupt patch at line 198"

Please rebase on master.

Trimming the patch a bit and comments inlined.
> -Original Message-
> From: Amber Hu 
> Sent: Tuesday, January 14, 2020 7:50 AM
> To: d...@openvswitch.org; Alin Serdean 
> Cc: Qiong Wang ; Wenyu Zhang
> ; Jinjun Gao 
> Subject: [patch] datapath-windows: Append tunnel info to upcall for 
correct
> template
> 

> diff --git a/datapath-windows/ovsext/Actions.c b/datapath-
> windows/ovsext/Actions.c
> 
> index 0c4d64b..936e68b 100644
> 
> --- a/datapath-windows/ovsext/Actions.c
> 
> +++ b/datapath-windows/ovsext/Actions.c
> 
> @@ -1839,11 +1839,16 @@ OvsOutputUserspaceAction(OvsForwardingContext
> *ovsFwdCtx,
> 
>   const PNL_ATTR attr)
> 
> {
> 
>  NTSTATUS status = NDIS_STATUS_SUCCESS;
> 
> -PNL_ATTR userdataAttr;
> 
> -PNL_ATTR queueAttr;
 [Alin Serdean] queueAttr can be safely removed since it's not actually 
used.
> 
> +PNL_ATTR userdataAttr = NULL;
> 
> +PNL_ATTR queueAttr = NULL;
> 
> +PNL_ATTR egrTunAttr = NULL;
> 
> +PNL_ATTR a = NULL;
[Alin Serdean] Not sure why did is needed. NlAttrFindNested does the safe 
version
of NL_ATTR_FOR_EACH_UNSAFE. Any reason to change it?
> 
>  POVS_PACKET_QUEUE_ELEM elem;
> 
>  POVS_PACKET_HDR_INFO layers = >layers;
> 
>  BOOLEAN isRecv = FALSE;
> 
> +INT rem = 0;
> 
> +OVS_FWD_INFO fwdInfo;
> 
> +OvsIPv4TunnelKey tunKey;
> 
>  POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(ovsFwdCtx-
> >switchContext,
> 
>ovsFwdCtx->srcVportNo);
> 
> @@ -1855,13 +1860,41 @@ OvsOutputUserspaceAction(OvsForwardingContext
> *ovsFwdCtx,
> 
>  }
> 
>  }
> 
> -queueAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_PID);
> 
> -userdataAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_USERDATA);
> 
> +NL_ATTR_FOR_EACH_UNSAFE(a, rem, NlAttrData(attr), 
NlAttrGetSize(attr)) {
[Alin Serdean] NlAttrFindNested does the safe version of 
NL_ATTR_FOR_EACH_UNSAFE.
Any reason to change it?
> 
> +switch (NlAttrType(a)) {
> 
> +case OVS_USERSPACE_ATTR_PID:
> 
> +queueAttr = a;
> 
> +break;
> 
> +case OVS_USERSPACE_ATTR_USERDATA:
> 
> +userdataAttr = a;
> 
> +break;
> 
> +case OVS_USERSPACE_ATTR_EGRESS_TUN_PORT:
> 
> +/* Indicate the packet is from egress tunnel port */
> 
> +egrTunAttr = a;
> 
> +break;
> 
> +default:
> 
> +break;
> 
> +}
> 
> +}
> 
> +
> 
> +/* Fill tunnel key to export to usersspace to calculate the template 
id */
> 
> +if (egrTunAttr) {
> 
[Alin Serdean] It would be safer to zero out tunKey before this.
> +RtlCopyMemory(, >tunKey, sizeof tunKey);
> 
> +if (!tunKey.src) {
> 
> +status = OvsLookupIPFwdInfo(tunKey.src, tunKey.dst, 
);
> 
> +if (status == NDIS_STATUS_SUCCESS && tunKey.dst ==
> fwdInfo.dstIpAddr) {
> 
> +tunKey.src = fwdInfo.srcIpAddr;
> 
> +}
> 
> +}
> 
> +tunKey.flow_hash = tunKey.flow_hash?tunKey.flow_hash:MAXINT16;
[Alin Serdean] Missing a whitespace before and after '?'
> 
> +}
> 
>  elem = OvsCreateQueueNlPacket(NlAttrData(userdataAttr),
> 
>NlAttrGetSize(userdataAttr),
> 
>OVS_PACKET_CMD_ACTION,
> 
> -  vport, key, ovsFwdCtx->curNbl,
> 
> +  vport, key,
> 
> +  egrTunAttr?&(tunKey):NULL,
[Alin Serdean] Missing whitespace before and after '?'
> 
> +  ovsFwdCtx->curNbl,
> 
>
NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl),
> 
>isRecv,
> 
>layers);
> 
> diff --git a/datapath-windows/ovsext/Flow.c b/datapath-
> windows/ovsext/Flow.c
> 
> index fdb1010..ac0582c 100644
> 
> --- a/datapath-windows/ovsext/Flow.c
> 
> +++ 

Re: [ovs-dev] [PATCH] Grant Access Privilege of Named Pipe to Creator

[Alin Serdean]

Hi,

Sorry I missed the email.

The direction sounds ok with me. It will surely help with unit tests, since 
right now
they require elevated permissions.

Adding also Anand in the loop.
Anand do you like the idea?

Please also add a few lines to the documentation so users are aware of the
change.

The patch as is, fails to apply. Rebase on master.

Also please change the title so it is in compliance with:
http://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/#email-subject
 

Thanks,
Alin.

> -Original Message-
> From: Ning Wu 
> Sent: Monday, January 20, 2020 4:16 AM
> To: d...@openvswitch.org; Alin Serdean 
> Cc: Lina Li ; Roy Luo 
> Subject: RE: [PATCH] Grant Access Privilege of Named Pipe to Creator
> 
> Hi Alin,
> 
> 
> 
> Could you please help to give some comment?
> 
> Thanks.
> 
> 
> 
> From: Ning Wu
> Sent: Friday, January 17, 2020 15:15
> To: d...@openvswitch.org; Alin Serdean 
> Cc: Lina Li ; Roy Luo 
> Subject: [PATCH] Grant Access Privilege of Named Pipe to Creator
> 
> 
> 
> Current implementation of ovs on windows only allows LocalSystem and
> 
> Administrators to access the named pipe created with API of ovs.
> 
> Thus any service that needs to invoke the API to create named pipe
> 
> has to run as System account to interactive with ovs. It causes the
> 
> system more vulnerable if one of those services was break into.
> 
> The patch adds the creator owner account to allowed ACLs.
> 
> 
> 
> Signed-off-by: Ning Wu mailto:n...@vmware.com> >
> 
> ---
> 
> lib/stream-windows.c | 33 -
> 
> 1 file changed, 32 insertions(+), 1 deletion(-)
> 
> 
> 
> diff --git a/lib/stream-windows.c b/lib/stream-windows.c
> 
> index 34bc610..0cad927 100644
> 
> --- a/lib/stream-windows.c
> 
> +++ b/lib/stream-windows.c
> 
> @@ -41,7 +41,7 @@ static void maybe_unlink_and_free(char *path);
> 
> #define LOCAL_PREFIX ".\\pipe\\  "
> 
> 
> 
> /* Size of the allowed PSIDs for securing Named Pipe. */
> 
> -#define ALLOWED_PSIDS_SIZE 2
> 
> +#define ALLOWED_PSIDS_SIZE 3
> 
> 
> 
> /* This function has the purpose to remove all the slashes received in s. */
> 
> static char *
> 
> @@ -412,6 +412,9 @@ create_pnpipe(char *name)
> 
>  PACL acl = NULL;
> 
>  PSECURITY_DESCRIPTOR psd = NULL;
> 
>  HANDLE npipe;
> 
> +HANDLE hToken = NULL;
> 
> +DWORD dwBufSize = 0;
> 
> +PTOKEN_USER pTokenUsr = NULL;
> 
> 
> 
>  /* Disable access over network. */
> 
>  if (!AllocateAndInitializeSid(, 1, SECURITY_NETWORK_RID,
> 
> @@ -438,6 +441,32 @@ create_pnpipe(char *name)
> 
>  goto handle_error;
> 
>  }
> 
> 
> 
> +/* Open the access token of calling process */
> 
> +if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, )) {
> 
> +   VLOG_ERR_RL(, "Error opening access token of calling process.");
> 
> +goto handle_error;
> 
> +}
> 
> +
> 
> +/* get the buffer size buffer needed for SID */
> 
> +GetTokenInformation(hToken, TokenUser, NULL, 0, );
> 
> +
> 
> +pTokenUsr = xmalloc(dwBufSize);
> 
> +memset(pTokenUsr, 0, dwBufSize);
> 
> +
> 
> +/* Retrieve the token information in a TOKEN_USER structure. */
> 
> +if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
> 
> +)) {
> 
> +VLOG_ERR_RL(, "Error retrieving token information.");
> 
> +goto handle_error;
> 
> +}
> 
> +CloseHandle(hToken);
> 
> +
> 
> +if (!IsValidSid(pTokenUsr->User.Sid)) {
> 
> +VLOG_ERR_RL(, "Invalid SID.");
> 
> +goto handle_error;
> 
> +}
> 
> +allowedPsid[2] = pTokenUsr->User.Sid;
> 
> +
> 
>  for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
> 
>  aclSize += sizeof(ACCESS_ALLOWED_ACE) +
> 
> GetLengthSid(allowedPsid[i]) -
> 
> @@ -490,11 +519,13 @@ create_pnpipe(char *name)
> 
>  npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX |
> FILE_FLAG_OVERLAPPED,
> 
>  PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | 
> PIPE_WAIT,
> 
>  64, BUFSIZE, BUFSIZE, 0, );
> 
> +free(pTokenUsr);
> 
>  free(acl);
> 
>  free(psd);
> 
>  return npipe;
> 
> 
> 
> handle_error:
> 
> +free(pTokenUsr);
> 
>  free(acl);
> 
>  free(psd);
> 
>  return INVALID_HANDLE_VALUE;
> 
> --
> 
> 2.6.2
> 
> 

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

[ovs-dev] Powerful Electr0nic M0squito Lamp ⚔️⚔️⚔️⚔️⚔️⚔️⚔️.........................................

[Mosquitron via dev]

/







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

Re: [ovs-dev] [patch] datapath-windows: Append tunnel info to upcall for correct template

[Alin Serdean]

Hi Amber,

The patch fails to apply:
"error: corrupt patch at line 198"

Please rebase on master.

Trimming the patch a bit and comments inlined.
> -Original Message-
> From: Amber Hu 
> Sent: Tuesday, January 14, 2020 7:50 AM
> To: d...@openvswitch.org; Alin Serdean 
> Cc: Qiong Wang ; Wenyu Zhang
> ; Jinjun Gao 
> Subject: [patch] datapath-windows: Append tunnel info to upcall for correct
> template
> 

> diff --git a/datapath-windows/ovsext/Actions.c b/datapath-
> windows/ovsext/Actions.c
> 
> index 0c4d64b..936e68b 100644
> 
> --- a/datapath-windows/ovsext/Actions.c
> 
> +++ b/datapath-windows/ovsext/Actions.c
> 
> @@ -1839,11 +1839,16 @@ OvsOutputUserspaceAction(OvsForwardingContext
> *ovsFwdCtx,
> 
>   const PNL_ATTR attr)
> 
> {
> 
>  NTSTATUS status = NDIS_STATUS_SUCCESS;
> 
> -PNL_ATTR userdataAttr;
> 
> -PNL_ATTR queueAttr;
 [Alin Serdean] queueAttr can be safely removed since it's not actually used.
> 
> +PNL_ATTR userdataAttr = NULL;
> 
> +PNL_ATTR queueAttr = NULL;
> 
> +PNL_ATTR egrTunAttr = NULL;
> 
> +PNL_ATTR a = NULL;
[Alin Serdean] Not sure why did is needed. NlAttrFindNested does the safe 
version
of NL_ATTR_FOR_EACH_UNSAFE. Any reason to change it?
> 
>  POVS_PACKET_QUEUE_ELEM elem;
> 
>  POVS_PACKET_HDR_INFO layers = >layers;
> 
>  BOOLEAN isRecv = FALSE;
> 
> +INT rem = 0;
> 
> +OVS_FWD_INFO fwdInfo;
> 
> +OvsIPv4TunnelKey tunKey;
> 
>  POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(ovsFwdCtx-
> >switchContext,
> 
>ovsFwdCtx->srcVportNo);
> 
> @@ -1855,13 +1860,41 @@ OvsOutputUserspaceAction(OvsForwardingContext
> *ovsFwdCtx,
> 
>  }
> 
>  }
> 
> -queueAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_PID);
> 
> -userdataAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_USERDATA);
> 
> +NL_ATTR_FOR_EACH_UNSAFE(a, rem, NlAttrData(attr), NlAttrGetSize(attr)) {
[Alin Serdean] NlAttrFindNested does the safe version of 
NL_ATTR_FOR_EACH_UNSAFE.
Any reason to change it?
> 
> +switch (NlAttrType(a)) {
> 
> +case OVS_USERSPACE_ATTR_PID:
> 
> +queueAttr = a;
> 
> +break;
> 
> +case OVS_USERSPACE_ATTR_USERDATA:
> 
> +userdataAttr = a;
> 
> +break;
> 
> +case OVS_USERSPACE_ATTR_EGRESS_TUN_PORT:
> 
> +/* Indicate the packet is from egress tunnel port */
> 
> +egrTunAttr = a;
> 
> +break;
> 
> +default:
> 
> +break;
> 
> +}
> 
> +}
> 
> +
> 
> +/* Fill tunnel key to export to usersspace to calculate the template id 
> */
> 
> +if (egrTunAttr) {
> 
[Alin Serdean] It would be safer to zero out tunKey before this.
> +RtlCopyMemory(, >tunKey, sizeof tunKey);
> 
> +if (!tunKey.src) {
> 
> +status = OvsLookupIPFwdInfo(tunKey.src, tunKey.dst, );
> 
> +if (status == NDIS_STATUS_SUCCESS && tunKey.dst ==
> fwdInfo.dstIpAddr) {
> 
> +tunKey.src = fwdInfo.srcIpAddr;
> 
> +}
> 
> +}
> 
> +tunKey.flow_hash = tunKey.flow_hash?tunKey.flow_hash:MAXINT16;
[Alin Serdean] Missing a whitespace before and after '?'
> 
> +}
> 
>  elem = OvsCreateQueueNlPacket(NlAttrData(userdataAttr),
> 
>NlAttrGetSize(userdataAttr),
> 
>OVS_PACKET_CMD_ACTION,
> 
> -  vport, key, ovsFwdCtx->curNbl,
> 
> +  vport, key,
> 
> +  egrTunAttr?&(tunKey):NULL,
[Alin Serdean] Missing whitespace before and after '?'
> 
> +  ovsFwdCtx->curNbl,
> 
>
> NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl),
> 
>isRecv,
> 
>layers);
> 
> diff --git a/datapath-windows/ovsext/Flow.c b/datapath-
> windows/ovsext/Flow.c
> 
> index fdb1010..ac0582c 100644
> 
> --- a/datapath-windows/ovsext/Flow.c
> 
> +++ b/datapath-windows/ovsext/Flow.c
> 
> @@ -1094,6 +1094,18 @@ MapFlowTunKeyToNlKey(PNL_BUFFER nlBuf,
> 
>  goto done;
> 
>  }
> 
> +if (!NlMsgPutTailU16(nlBuf, OVS_TUNNEL_KEY_ATTR_TP_SRC,
> 
> + tunKey->flow_hash)) {
> 
> +rc = STATUS_UNSUCCESSFUL;
> 
> +goto done;
> 
> +}
> 
> +
> 
> +if (!NlMsgPutTailU16(nlBuf, OVS_TUNNEL_KEY_ATTR_TP_DST,
> 
> + tunKey->dst_port)) {
> 
> +rc = STATUS_UNSUCCESSFUL;
> 
> +goto done;
> 
> +}
> 
> +
> 
> done:
> 
>  NlMsgEndNested(nlBuf, offset);
> 
> error_nested_start:
> 
> diff --git a/datapath-windows/ovsext/User.c b/datapath-
> windows/ovsext/User.c
> 
> index 16d6fd2..a1d05e1 100644
> 
> --- a/datapath-windows/ovsext/User.c
> 
> +++ b/datapath-windows/ovsext/User.c
> 

[ovs-dev] About your ATM shipment

[Anina Lylund]

Dear Friend,
I am Anina Lylund, the newly parcel delivery officer with the DHL Courier
Services Nigeria. During my personal audit yesterday, I came across an
abandoned registered ATM Visa Card bearing your name and address as the
recipient. After studying the shipment papers carefully, I discovered that
you did not complete the required shipment procedure by procuring an
insurance coverage on your parcel shipment; hence you have not received
your parcel up to this moment.  On this note, you are required to procure
an insurance paper to enable our delivery agents complete the delivery of
your ATM Parcel to your designated address without further delays.

 Meanwhile, you are to re-confirm your below personal details to avoid
dealing with a dead/wrong person.

1. Your full name
2. Your address
3. Your country name
4. Your direct cell phone number

We are sorry for delay and inconveniences.

We thank you in anticipation for your expected cooperation.

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

Re: [ovs-dev] [PATCH] ofproto: Fix for frequent invalidation of flows due to mismatch in mask bits

[Ben Pfaff]

On Thu, Jan 16, 2020 at 09:47:23PM +0530, Vishal Deep Ajmera via dev wrote:
> The wildcard bits in the installed megaflow entry could be different from
> the bits originally generated by the ofproto layer. Datapath implementation
> wildcards those match fields which are not present in the incoming packet
> before installing the flow.
> 
> When the revalidator thread validates a megaflow, it will first query the
> ofproto layer to get the wildcard bits and then compare it against the
> wildcard bits in the megaflow. If the bits are different the entry will be
> removed. A subsequent packet will again result in the same megaflow entry
> being installed only for it to be removed by the revalidator thread. This
> cycle will continue and will significantly degrade performance.
> 
> An earlier patch fixing the issue for MPLS and VLAN was sent.
> However similar problem now appears for IPv6 datapath flows.
> 
> This patch addresses the issue in a generic way.
> 
> Signed-off-by: Vishal Deep Ajmera 

This is clever!  I think that this kind of approach is the right one.

I don't yet understand why this should be moved from xlate_wc_finish()
to revalidate_ukey__().  Can you help me understand that?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ovn-northd: Set stage-hint for all applicable flows.

[Mark Michelson]

Hi Dumitru,

It appears that some of the stage hints correspond to DHCPv4_Options and 
DHCPv6_Options, but these are not handled in ovn-detrace.


On 1/17/20 10:44 AM, Dumitru Ceara wrote:

Until now the 'stage-hint' external-id was set only for logical flows
installed for ACLs. In order to simplify troubleshooting, extend the
approach and apply whenever possible. Set stage-hint for logical flows
generated by the following NB tables too:
- Logical_Switch_Port
- Logical_Router_Port
- Load_Balancer
- NAT

Also update ovn-detrace such that whenever stage-hints are available,
all the tables mentioned above are queried and if the stage-hint matches
a NB uuid, relevant information is dumped.

Signed-off-by: Dumitru Ceara 
---
  northd/ovn-northd.c  | 721 +--
  utilities/ovn-detrace.in |  93 --
  2 files changed, 524 insertions(+), 290 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index b6dc809..20b8429 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -3714,6 +3714,15 @@ ovn_lflow_hash(const struct ovn_lflow *lflow)
   lflow->actions);
  }
  
+static char *

+ovn_lflow_hint(const struct ovsdb_idl_row *row)
+{
+if (!row) {
+return NULL;
+}
+return xasprintf("%08x", row->uuid.parts[0]);
+}
+
  static bool
  ovn_lflow_equal(const struct ovn_lflow *a, const struct ovn_lflow *b)
  {
@@ -3744,14 +3753,14 @@ static void
  ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
   enum ovn_stage stage, uint16_t priority,
   const char *match, const char *actions,
- const char *stage_hint, const char *where)
+ const struct ovsdb_idl_row *stage_hint, const char *where)
  {
  ovs_assert(ovn_stage_to_datapath_type(stage) == 
ovn_datapath_get_type(od));
  
  struct ovn_lflow *lflow = xmalloc(sizeof *lflow);

  ovn_lflow_init(lflow, od, stage, priority,
 xstrdup(match), xstrdup(actions),
-   nullable_xstrdup(stage_hint), where);
+   ovn_lflow_hint(stage_hint), where);
  hmap_insert(lflow_map, >hmap_node, ovn_lflow_hash(lflow));
  }
  
@@ -3902,7 +3911,8 @@ build_port_security_ipv6_flow(

   *   - Priority 80 flow to drop ARP and IPv6 ND packets.
   */
  static void
-build_port_security_nd(struct ovn_port *op, struct hmap *lflows)
+build_port_security_nd(struct ovn_port *op, struct hmap *lflows,
+   const struct ovsdb_idl_row *stage_hint)
  {
  struct ds match = DS_EMPTY_INITIALIZER;
  
@@ -3938,8 +3948,8 @@ build_port_security_nd(struct ovn_port *op, struct hmap *lflows)

  ds_chomp(, ',');
  ds_put_cstr(, "}");
  }
-ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_ND, 90,
-  ds_cstr(), "next;");
+ovn_lflow_add_with_hint(lflows, op->od, S_SWITCH_IN_PORT_SEC_ND,
+90, ds_cstr(), "next;", stage_hint);
  }
  
  if (ps->n_ipv6_addrs || no_ip) {

@@ -3948,15 +3958,15 @@ build_port_security_nd(struct ovn_port *op, struct hmap 
*lflows)
op->json_key, ps->ea_s);
  build_port_security_ipv6_nd_flow(, ps->ea, ps->ipv6_addrs,
   ps->n_ipv6_addrs);
-ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_ND, 90,
-  ds_cstr(), "next;");
+ovn_lflow_add_with_hint(lflows, op->od, S_SWITCH_IN_PORT_SEC_ND,
+90, ds_cstr(), "next;", stage_hint);
  }
  }
  
  ds_clear();

  ds_put_format(, "inport == %s && (arp || nd)", op->json_key);
-ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_ND, 80,
-  ds_cstr(), "drop;");
+ovn_lflow_add_with_hint(lflows, op->od, S_SWITCH_IN_PORT_SEC_ND, 80,
+ds_cstr(), "drop;", stage_hint);
  ds_destroy();
  }
  
@@ -3977,7 +3987,8 @@ build_port_security_nd(struct ovn_port *op, struct hmap *lflows)

   */
  static void
  build_port_security_ip(enum ovn_pipeline pipeline, struct ovn_port *op,
-   struct hmap *lflows)
+   struct hmap *lflows,
+   const struct ovsdb_idl_row *stage_hint)
  {
  char *port_direction;
  enum ovn_stage stage;
@@ -4007,8 +4018,9 @@ build_port_security_ip(enum ovn_pipeline pipeline, struct 
ovn_port *op,
" && ip4.dst == 255.255.255.255"
" && udp.src == 68 && udp.dst == 67",
op->json_key, ps->ea_s);
-ovn_lflow_add(lflows, op->od, stage, 90,
-  ds_cstr(_match), "next;");
+ovn_lflow_add_with_hint(lflows, op->od, stage, 90,
+ds_cstr(_match), "next;",
+

Re: [ovs-dev] [PATCH v5] userspace: Add TCP Segmentation Offload support

[Ben Pfaff]

On Sat, Jan 18, 2020 at 12:08:06AM +0100, Ilya Maximets wrote:
> On 18.01.2020 00:03, Stokes, Ian wrote:
> > Thanks all for review/testing, pushed to master.
> 
> OK, thanks Ian.
> 
> @Ben, even though this patch already merged, I'd ask you to take a look
> at the code in case you'll spot some issues especially in non-DPDK related
> parts.

I found the name dp_packet_hwol_is_ipv4(), and similar, confusing.  The
name suggested to me "test whether the packet is IPv4" not "test whether
the packet has an offloaded IPv4 checksum".  I guess the "hwol" is
offload related but...  I like the name dp_packet_hwol_tx_l4_checksum()
much more, it makes it obvious at a glance that it's a
checksum-offloading check.

In the case where we actually receive a 64 kB packet, I think that this
code is going to be relatively inefficient.  If I'm reading the code
correctly (I did it quickly), then this is what happens:

- The first 1500 bytes of the packet land in the first
  dp_packet.

- The remaining 64000ish bytes land in the second dp_packet.

- Then we expand the first dp_packet to the needed size and copy
  the remaining 64000 bytes into it.

An alternative would be:

- Set up the first dp_packet as currently.

- Set up the second dp_packet so that the bytes are received
  into it starting at offset (mtu + headroom).

- If more than mtu bytes are received, then copy those bytes
  into the headroom of the second dp_packet and return it to the
  caller instead of the first dp_packet.

The advantage is that we do a 1500-byte copy instead of a 64000-byte
copy.  The disadvantage is that we waste any memory leftover in the
second dp_packet, e.g. 32 kB if it's only a 32 kB packet instead of 64
kB.  Also we need slightly more sophisticated dp_packet allocation (we
will need to replenish the supply of aux_bufs).

Thanks,

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

Re: [ovs-dev] [PATCH v2 1/2] Prepare for 2.13.0.

[0-day Robot]

Bleep bloop.  Greetings Ben Pfaff, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
Failed to merge in the changes.
Patch failed at 0001 Prepare for 2.13.0.
The copy of the patch that failed is found in:
   
/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Prepare for 2.13.0.

[0-day Robot]

Bleep bloop.  Greetings Ben Pfaff, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
Failed to merge in the changes.
Patch failed at 0001 Prepare for 2.13.0.
The copy of the patch that failed is found in:
   
/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 2/2] Prepare for post-2.13.0 (2.13.90).

[Ben Pfaff]

On Tue, Jan 21, 2020 at 12:55:17PM -0800, Guru Shetty wrote:
> On Tue, 21 Jan 2020 at 12:46, Ben Pfaff  wrote:
> 
> > Signed-off-by: Ben Pfaff 
> >
> For both:
> Acked-by: Gurucharan Shetty 

Thank you!

We are now branched for the 2.13 release.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v10 1/1] Avoid dp_hash recirculation for balance-tcp bond selection mode

[Ben Pfaff]

On Mon, Jan 20, 2020 at 05:09:05PM +0100, Ilya Maximets wrote:
> > +static struct tx_bond *
> > +tx_bond_lookup(const struct hmap *hmap, uint32_t bond_id)
> > +{
> > +struct tx_bond *tx;
> > +
> > +HMAP_FOR_EACH_IN_BUCKET (tx, node, hash_bond_id(bond_id), hmap) {
> 
> Why not HMAP_FOR_EACH_WITH_HASH ?

HMAP_FOR_EACH_IN_BUCKET is a small optimization over
HMAP_FOR_EACH_WITH_HASH in cases where the comparison of the key is
cheap.  Comparing the hash is an optimization if the key is expensive to
compare (like an arbitrary string) but it just costs an extra integer
comparison if the key is a small integer as here.

> BTW, it's better to calculate hash before the loop to be sure that
> we're not recalculating it on each iteration.

A lot of code assumes that the hash only gets evaluated once.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 2/2] Prepare for post-2.13.0 (2.13.90).

[Guru Shetty]

On Tue, 21 Jan 2020 at 12:46, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
>
For both:
Acked-by: Gurucharan Shetty 


> ---
>  NEWS | 4 
>  configure.ac | 2 +-
>  debian/changelog | 6 ++
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/NEWS b/NEWS
> index 89b53bfae039..100cb22d568d 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,3 +1,7 @@
> +Post-v2.13.0
> +-
> +
> +
>  v2.13.0 - xx xxx 
>  -
> - OVN:
> diff --git a/configure.ac b/configure.ac
> index 92b52f67127e..1877aae561d8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -13,7 +13,7 @@
>  # limitations under the License.
>
>  AC_PREREQ(2.63)
> -AC_INIT(openvswitch, 2.13.0, b...@openvswitch.org)
> +AC_INIT(openvswitch, 2.13.90, b...@openvswitch.org)
>  AC_CONFIG_SRCDIR([datapath/datapath.c])
>  AC_CONFIG_MACRO_DIR([m4])
>  AC_CONFIG_AUX_DIR([build-aux])
> diff --git a/debian/changelog b/debian/changelog
> index c6db87d542d9..4ec058d99e28 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,3 +1,9 @@
> +openvswitch (2.13.90-1) unstable; urgency=low
> +
> +   * New upstream version
> +
> + -- Open vSwitch team   Tue, 21 Jan 2020 12:44:30
> -0700
> +
>  openvswitch (2.13.0-1) unstable; urgency=low
>
> * New upstream version
> --
> 2.23.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 0/2] Branch for 2.13.0

[Ben Pfaff]

v2 since I forgot the second patch the first time (thanks Guru!).

Ben Pfaff (2):
  Prepare for 2.13.0.
  Prepare for post-2.13.0 (2.13.90).

 NEWS |  6 +-
 configure.ac |  2 +-
 debian/changelog | 10 --
 3 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.23.0

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

Re: [ovs-dev] [PATCH] Prepare for 2.13.0.

[Ben Pfaff]

On Tue, Jan 21, 2020 at 12:42:55PM -0800, Guru Shetty wrote:
> On Tue, 21 Jan 2020 at 12:27, Ben Pfaff  wrote:
> 
> > Signed-off-by: Ben Pfaff 
> >
> 
> Usually I see one more "Prepare for post-" patch. But I guess, you intend
> to do it later.

Oh, you're right.  Usually I rely on Justin to do these, but that's not
going to happen anymore.

I sent v2 adding the second patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 1/2] Prepare for 2.13.0.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 NEWS | 2 +-
 configure.ac | 2 +-
 debian/changelog | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index c6d3b6053f8d..89b53bfae039 100644
--- a/NEWS
+++ b/NEWS
@@ -1,4 +1,4 @@
-Post-v2.12.0
+v2.13.0 - xx xxx 
 -
- OVN:
  * OVN has been removed from this repository. It now exists as a
diff --git a/configure.ac b/configure.ac
index 4f483fa6e3c3..92b52f67127e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 AC_PREREQ(2.63)
-AC_INIT(openvswitch, 2.12.90, b...@openvswitch.org)
+AC_INIT(openvswitch, 2.13.0, b...@openvswitch.org)
 AC_CONFIG_SRCDIR([datapath/datapath.c])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
diff --git a/debian/changelog b/debian/changelog
index 9042657ef717..c6db87d542d9 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,8 +1,8 @@
-openvswitch (2.12.90-1) unstable; urgency=low
+openvswitch (2.13.0-1) unstable; urgency=low
 
* New upstream version
 
- -- Open vSwitch team   Mon, 22 Jul 2019 09:38:30 -0700
+ -- Open vSwitch team   Tue, 21 Jan 2020 12:24:09 -0700
 
 openvswitch (2.12.0-1) unstable; urgency=low
[ Open vSwitch team ]
-- 
2.23.0

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

[ovs-dev] [PATCH v2 2/2] Prepare for post-2.13.0 (2.13.90).

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 NEWS | 4 
 configure.ac | 2 +-
 debian/changelog | 6 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 89b53bfae039..100cb22d568d 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,7 @@
+Post-v2.13.0
+-
+
+
 v2.13.0 - xx xxx 
 -
- OVN:
diff --git a/configure.ac b/configure.ac
index 92b52f67127e..1877aae561d8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 AC_PREREQ(2.63)
-AC_INIT(openvswitch, 2.13.0, b...@openvswitch.org)
+AC_INIT(openvswitch, 2.13.90, b...@openvswitch.org)
 AC_CONFIG_SRCDIR([datapath/datapath.c])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
diff --git a/debian/changelog b/debian/changelog
index c6db87d542d9..4ec058d99e28 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+openvswitch (2.13.90-1) unstable; urgency=low
+
+   * New upstream version
+
+ -- Open vSwitch team   Tue, 21 Jan 2020 12:44:30 -0700
+
 openvswitch (2.13.0-1) unstable; urgency=low
 
* New upstream version
-- 
2.23.0

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

Re: [ovs-dev] [PATCH ovn v2 0/2] Caching logical flow expr tree for each lflow

[Mark Michelson]

I have pushed the series to master.

On 1/21/20 3:05 PM, Han Zhou wrote:

On Tue, Jan 21, 2020 at 6:40 AM  wrote:


From: Numan Siddique 

This patch series tries to improve the time taken to proceess logical
flows by caching the expr tree. For large scale deployments with lots
of logical flows, the logical flow processing to OpenFlow rules
takes a lot of time as it is CPU intensive.

This patch series tries to improve this by caching the expr tree
generated for each logical flow so that we don't have to generate the
expr tree for each lflow_run().

Below are the details of the OVN resource in my setup

No of logical switches - 49
No of logical ports- 1191
No of logical routers  - 7
No of logical ports- 51
No of ACLs - 1221
No of Logical flows- 664736

On a chassis hosting 7 distributed router ports and around 1090
port bindings, the no of OVS rules was 921162.

Without this patch, the function add_logical_flows() took ~15 seconds.
And with this patch it took ~10 seconds. There is a good 34% improvement
in logical flow processing.

v1 -> v2
===
  * Addressed review comments from Han.

Numan Siddique (2):
   expr: Evaluate the condition expression in a separate step.
   ovn-controller: Cache logical flow expr tree for each lflow.

  controller/lflow.c  | 191 +++-
  controller/lflow.h  |   9 +-
  controller/ovn-controller.c |  16 ++-
  include/ovn/expr.h  |  10 +-
  lib/expr.c  |  55 ---
  tests/test-ovn.c|  10 +-
  utilities/ovn-trace.c   |   5 +-
  7 files changed, 218 insertions(+), 78 deletions(-)

--
2.24.1

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


For v2 of the series:
Acked-by: Han Zhou 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev



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

Re: [ovs-dev] [PATCH] Prepare for 2.13.0.

[Guru Shetty]

On Tue, 21 Jan 2020 at 12:27, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
>

Usually I see one more "Prepare for post-" patch. But I guess, you intend
to do it later.

Acked-by: Guru Shetty 

> ---
>  NEWS | 2 +-
>  configure.ac | 2 +-
>  debian/changelog | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index c6d3b6053f8d..89b53bfae039 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,4 +1,4 @@
> -Post-v2.12.0
> +v2.13.0 - xx xxx 
>  -
> - OVN:
>   * OVN has been removed from this repository. It now exists as a
> diff --git a/configure.ac b/configure.ac
> index 4f483fa6e3c3..92b52f67127e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -13,7 +13,7 @@
>  # limitations under the License.
>
>  AC_PREREQ(2.63)
> -AC_INIT(openvswitch, 2.12.90, b...@openvswitch.org)
> +AC_INIT(openvswitch, 2.13.0, b...@openvswitch.org)
>  AC_CONFIG_SRCDIR([datapath/datapath.c])
>  AC_CONFIG_MACRO_DIR([m4])
>  AC_CONFIG_AUX_DIR([build-aux])
> diff --git a/debian/changelog b/debian/changelog
> index 9042657ef717..c6db87d542d9 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,8 +1,8 @@
> -openvswitch (2.12.90-1) unstable; urgency=low
> +openvswitch (2.13.0-1) unstable; urgency=low
>
> * New upstream version
>
> - -- Open vSwitch team   Mon, 22 Jul 2019 09:38:30
> -0700
> + -- Open vSwitch team   Tue, 21 Jan 2020 12:24:09
> -0700
>
>  openvswitch (2.12.0-1) unstable; urgency=low
> [ Open vSwitch team ]
> --
> 2.23.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch] datapath-windows: Append tunnel info to upcall for correct template

[Alin Serdean]

Sure. I will review it tonight.

Thanks,
Alin.

-Original Message-
From: Ben Pfaff  
Sent: Tuesday, January 21, 2020 7:59 PM
To: Amber Hu 
Cc: d...@openvswitch.org; Alin Serdean ; Qiong 
Wang 
Subject: Re: [ovs-dev] [patch] datapath-windows: Append tunnel info to upcall 
for correct template

I expect that, of the OVS maintainers, Alin is most qualified to review it.  
Alin, are you planning to take a look at it?

On Tue, Jan 21, 2020 at 02:52:54AM +, Amber Hu via dev wrote:
> Hi,
> 
> May I ask the process of reviewing for the below patch 
> https://patchwork.ozlabs.org/patch/1222463/
> Thanks for your time.
> 
> BR/Amber
> 
> From: Amber Hu 
> Date: Tuesday, January 14, 2020 at 13:49
> To: "d...@openvswitch.org" , Alin Serdean 
> 
> Cc: Qiong Wang , Wenyu Zhang , 
> Jinjun Gao 
> Subject: [patch] datapath-windows: Append tunnel info to upcall for 
> correct template
> 
> Formerly, there is no tunnel information appended in the upcall’s 
> packet data, which is expected by IPFIX in userspace to calculate the 
> template for exporting the sampled flow record of on egress tunnel 
> port.
> To fix this, during performing OvsOutputUserspaceAction(), we would 
> check whether it is initiated by the sampling on egress tunnel which 
> would be indicated by the attribute as 
> OVS_USERSPACE_ATTR_EGRESS_TUN_PORT in the nested attribute list. If 
> so, we would append the tunKey in OvsForwardingContext indexed by 
> OVS_PACKET_ATTR_EGRESS_TUN_KEY to the upcall.
> Besides, at this point, the source transport port and  source ip 
> address are not available in the structure, so we have to fill it in 
> the way how the packet would be capsulated during performing 
> OvsEncapGeneve(), which is following the
> OvsOutputUserspaceAction() unfortunately.
> I have tested the IPFIX functionality with the change, we could see 
> the template is correct and the expected tunnel information could be 
> packed in the IPFIX packet finally. The traffic for test is generated 
> by PING utility.
> 
> Issue: 2449045
> Reported-by: Meng Yang mailto:ym...@vmware.com>>
> Reported-at: https://bugzilla.eng.vmware.com/show_bug.cgi?id=2449045
> Signed-off-by: Amber Hu 
> 
> From 8262494fb9a353c3adbe02cfc4c932231c34b3ed Mon Sep 17 00:00:00 2001
> From: Amber Hu 
> Date: Sun, 12 Jan 2020 22:40:05 -0800
> Subject: [PATCH] datapath-windows: Append tunnel info to upcall for 
> correct template
> 
> Issue: #2449045
> Signed-off-by: Amber Hu 
> ---
> datapath-windows/ovsext/Actions.c | 43 ++-
> datapath-windows/ovsext/Flow.c| 12 +++
> datapath-windows/ovsext/User.c| 10 +++--
> datapath-windows/ovsext/User.h|  1 +
> 4 files changed, 59 insertions(+), 7 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/Actions.c 
> b/datapath-windows/ovsext/Actions.c
> index 0c4d64b..936e68b 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -1839,11 +1839,16 @@ OvsOutputUserspaceAction(OvsForwardingContext 
> *ovsFwdCtx,
>   const PNL_ATTR attr) {
>  NTSTATUS status = NDIS_STATUS_SUCCESS;
> -PNL_ATTR userdataAttr;
> -PNL_ATTR queueAttr;
> +PNL_ATTR userdataAttr = NULL;
> +PNL_ATTR queueAttr = NULL;
> +PNL_ATTR egrTunAttr = NULL;
> +PNL_ATTR a = NULL;
>  POVS_PACKET_QUEUE_ELEM elem;
>  POVS_PACKET_HDR_INFO layers = >layers;
>  BOOLEAN isRecv = FALSE;
> +INT rem = 0;
> +OVS_FWD_INFO fwdInfo;
> +OvsIPv4TunnelKey tunKey;
>  POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(ovsFwdCtx->switchContext,
>
> ovsFwdCtx->srcVportNo); @@ -1855,13 +1860,41 @@ 
> OvsOutputUserspaceAction(OvsForwardingContext *ovsFwdCtx,
>  }
>  }
> -queueAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_PID);
> -userdataAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_USERDATA);
> +NL_ATTR_FOR_EACH_UNSAFE(a, rem, NlAttrData(attr), NlAttrGetSize(attr)) {
> +switch (NlAttrType(a)) {
> +case OVS_USERSPACE_ATTR_PID:
> +queueAttr = a;
> +break;
> +case OVS_USERSPACE_ATTR_USERDATA:
> +userdataAttr = a;
> +break;
> +case OVS_USERSPACE_ATTR_EGRESS_TUN_PORT:
> +/* Indicate the packet is from egress tunnel port */
> +egrTunAttr = a;
> +break;
> +default:
> +break;
> +}
> +}
> +
> +/* Fill tunnel key to export to usersspace to calculate the template id 
> */
> +if (egrTunAttr) {
> +RtlCopyMemory(, >tunKey, sizeof tunKey);
> +if (!tunKey.src) {
> +status = OvsLookupIPFwdInfo(tunKey.src, tunKey.dst, );
> +if (status == NDIS_STATUS_SUCCESS && tunKey.dst == 
> fwdInfo.dstIpAddr) {
> +tunKey.src = fwdInfo.srcIpAddr;
> +}
> +}
> +tunKey.flow_hash = 

[ovs-dev] [PATCH] Prepare for 2.13.0.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 NEWS | 2 +-
 configure.ac | 2 +-
 debian/changelog | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index c6d3b6053f8d..89b53bfae039 100644
--- a/NEWS
+++ b/NEWS
@@ -1,4 +1,4 @@
-Post-v2.12.0
+v2.13.0 - xx xxx 
 -
- OVN:
  * OVN has been removed from this repository. It now exists as a
diff --git a/configure.ac b/configure.ac
index 4f483fa6e3c3..92b52f67127e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 AC_PREREQ(2.63)
-AC_INIT(openvswitch, 2.12.90, b...@openvswitch.org)
+AC_INIT(openvswitch, 2.13.0, b...@openvswitch.org)
 AC_CONFIG_SRCDIR([datapath/datapath.c])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
diff --git a/debian/changelog b/debian/changelog
index 9042657ef717..c6db87d542d9 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,8 +1,8 @@
-openvswitch (2.12.90-1) unstable; urgency=low
+openvswitch (2.13.0-1) unstable; urgency=low
 
* New upstream version
 
- -- Open vSwitch team   Mon, 22 Jul 2019 09:38:30 -0700
+ -- Open vSwitch team   Tue, 21 Jan 2020 12:24:09 -0700
 
 openvswitch (2.12.0-1) unstable; urgency=low
[ Open vSwitch team ]
-- 
2.23.0

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

Re: [ovs-dev] [PATCH ovn v2 0/2] Caching logical flow expr tree for each lflow

[Han Zhou]

On Tue, Jan 21, 2020 at 6:40 AM  wrote:
>
> From: Numan Siddique 
>
> This patch series tries to improve the time taken to proceess logical
> flows by caching the expr tree. For large scale deployments with lots
> of logical flows, the logical flow processing to OpenFlow rules
> takes a lot of time as it is CPU intensive.
>
> This patch series tries to improve this by caching the expr tree
> generated for each logical flow so that we don't have to generate the
> expr tree for each lflow_run().
>
> Below are the details of the OVN resource in my setup
>
> No of logical switches - 49
> No of logical ports- 1191
> No of logical routers  - 7
> No of logical ports- 51
> No of ACLs - 1221
> No of Logical flows- 664736
>
> On a chassis hosting 7 distributed router ports and around 1090
> port bindings, the no of OVS rules was 921162.
>
> Without this patch, the function add_logical_flows() took ~15 seconds.
> And with this patch it took ~10 seconds. There is a good 34% improvement
> in logical flow processing.
>
> v1 -> v2
> ===
>  * Addressed review comments from Han.
>
> Numan Siddique (2):
>   expr: Evaluate the condition expression in a separate step.
>   ovn-controller: Cache logical flow expr tree for each lflow.
>
>  controller/lflow.c  | 191 +++-
>  controller/lflow.h  |   9 +-
>  controller/ovn-controller.c |  16 ++-
>  include/ovn/expr.h  |  10 +-
>  lib/expr.c  |  55 ---
>  tests/test-ovn.c|  10 +-
>  utilities/ovn-trace.c   |   5 +-
>  7 files changed, 218 insertions(+), 78 deletions(-)
>
> --
> 2.24.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

For v2 of the series:
Acked-by: Han Zhou 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2 2/2] ovn-controller: Cache logical flow expr tree for each lflow.

[Han Zhou]

On Tue, Jan 21, 2020 at 6:41 AM  wrote:
>
> From: Numan Siddique 
>
> This patch caches the logical flow expr tree for each logical flow. This
> cache is stored as an hashmap in the output_flow engine data. When a
logical
> flow is deleted, the expr tree for that lflow is deleted in the
> flow_output_sb_logical_flow_handler().
>
> Below are the details of the OVN resource in my setup
>
> No of logical switches - 49
> No of logical ports- 1191
> No of logical routers  - 7
> No of logical ports- 51
> No of ACLs - 1221
> No of Logical flows- 664736
>
> On a chassis hosting 7 distributed router ports and around 1090
> port bindings, the no of OVS rules was 921162.
>
> Without this patch, the function add_logical_flows() took ~15 seconds.
> And with this patch it took ~10 seconds. There is a good 34% improvement
> in logical flow processing.
>
> Acked-by: Mark Michelson 
> Signed-off-by: Numan Siddique 
> ---
>  controller/lflow.c  | 192 ++--
>  controller/lflow.h  |   9 +-
>  controller/ovn-controller.c |  16 ++-
>  3 files changed, 160 insertions(+), 57 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 93ec53a1c..997c59662 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -75,11 +75,13 @@ static bool consider_logical_flow(
>  const struct shash *port_groups,
>  const struct sset *active_tunnels,
>  const struct sset *local_lport_ids,
> +bool delete_expr_from_cache,
>  struct ovn_desired_flow_table *,
>  struct ovn_extend_table *group_table,
>  struct ovn_extend_table *meter_table,
>  struct lflow_resource_ref *lfrr,
> -uint32_t *conj_id_ofs);
> +uint32_t *conj_id_ofs,
> +struct hmap *lflow_expr_cache);
>
>  static bool
>  lookup_port_cb(const void *aux_, const char *port_name, unsigned int
*portp)
> @@ -255,6 +257,66 @@ lflow_resource_destroy_lflow(struct
lflow_resource_ref *lfrr,
>  free(lfrn);
>  }
>
> +/* Represents expr tree for the lflow. We don't store the
> + * match in this structure because -
> + *   - Whenever a flow match or action needs to be modified,
> + * ovn-northd deletes the existing flow in the logical_flow
> + * table and adds a new one.
> + *  We may want to revisit this and store match incase the behavior
> + *  of ovn-northd changes.
> + */
> +struct lflow_expr {
> +struct hmap_node node;
> +struct uuid lflow_uuid; /* key */
> +struct expr *expr;
> +};
> +
> +static void
> +lflow_expr_add(struct hmap *lflow_expr_cache,
> +   const struct sbrec_logical_flow *lflow,
> +   struct expr *lflow_expr)
> +{
> +struct lflow_expr *le = xmalloc(sizeof *le);
> +le->lflow_uuid = lflow->header_.uuid;
> +le->expr = lflow_expr;
> +hmap_insert(lflow_expr_cache, >node, uuid_hash(>lflow_uuid));
> +}
> +
> +static struct lflow_expr *
> +lflow_expr_get(struct hmap *lflow_expr_cache,
> +   const struct sbrec_logical_flow *lflow)
> +{
> +struct lflow_expr *le;
> +size_t hash = uuid_hash(>header_.uuid);
> +HMAP_FOR_EACH_WITH_HASH (le, node, hash, lflow_expr_cache) {
> +if (uuid_equals(>lflow_uuid, >header_.uuid)) {
> +return le;
> +}
> +}
> +
> +return NULL;
> +}
> +
> +static void
> +lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
> +{
> +hmap_remove(lflow_expr_cache, >node);
> +expr_destroy(le->expr);
> +free(le);
> +}
> +
> +void
> +lflow_expr_destroy(struct hmap *lflow_expr_cache)
> +{
> +struct lflow_expr *le;
> +HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
> +expr_destroy(le->expr);
> +free(le);
> +}
> +
> +hmap_destroy(lflow_expr_cache);
> +}
> +
>  /* Adds the logical flows from the Logical_Flow table to flow tables. */
>  static void
>  add_logical_flows(
> @@ -273,7 +335,8 @@ add_logical_flows(
>  struct ovn_extend_table *group_table,
>  struct ovn_extend_table *meter_table,
>  struct lflow_resource_ref *lfrr,
> -uint32_t *conj_id_ofs)
> +uint32_t *conj_id_ofs,
> +struct hmap *lflow_expr_cache)
>  {
>  const struct sbrec_logical_flow *lflow;
>
> @@ -306,9 +369,9 @@ add_logical_flows(
> chassis, _opts, _opts,
> _ra_opts, _event_opts,
> addr_sets, port_groups,
> -   active_tunnels, local_lport_ids,
> +   active_tunnels, local_lport_ids,
false,
> flow_table, group_table, meter_table,
> -   lfrr, conj_id_ofs)) {
> +   lfrr, conj_id_ofs, lflow_expr_cache))
{
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
5);
>  VLOG_ERR_RL(, "Conjunction id overflow when processing
lflow "
>  UUID_FMT, 

Re: [ovs-dev] [PATCH] Use TPACKET_V1/V2/V3 to accelerate veth for DPDK datapath

[Ben Pfaff]

On Tue, Jan 21, 2020 at 02:49:47AM -0500, yang_y...@163.com wrote:
> From: Yi Yang 
> 
> We can avoid high system call overhead by using TPACKET_V1/V2/V3
> and use DPDK-like poll to receive and send packets (Note: send
> still needs to call sendto to trigger final packet transmission).
> 
> I can see about 30% improvement compared to last recvmmsg
> optimization if I use TPACKET_V3. TPACKET_V1/V2 is worse than
> TPACKET_V3, but it still can improve about 20%.
> 
> For veth, it is 1.47 Gbps before this patch, it is about 1.98
> Gbps after applied this patch. But it is about 4.00 Gbps if we
> use af_packet for veth, the bottle neck lies in ovs-vswitchd
> thread, it will handle too many things for every loop (as below)
> , so it can't work very efficintly as pmd_thread.
> 
> memory_run();
> bridge_run();
> unixctl_server_run(unixctl);
> netdev_run();
> 
> memory_wait();
> bridge_wait();
> unixctl_server_wait(unixctl);
> netdev_wait();
> poll_block();
> 
> In the next step, it will be better if let pmd_thread to handle
> tap and veth interface.
> 
> Signed-off-by: Yi Yang 
> Co-authored-by: William Tu 
> Signed-off-by: William Tu 

Thanks for the patch!

I am a bit concerned about version compatibility issues here.  There are
two relevant kinds of versions.  The first is the version of the
kernel/library headers.  This patch works pretty hard to adapt to the
headers that are available at compile time, only dealing with the
versions of the protocols that are available from the headers.  This
approach is sometimes fine, but an approach can be better is to simply
declare the structures or constants that the headers lack.  This is
often pretty easy for Linux data structures.  OVS does this for some
structures that it cares about with the headers in ovs/include/linux.
This approach has two advantages: the OVS code (outside these special
declarations) doesn't have to care whether particular structures are
declared, because they are always declared, and the OVS build always
supports a particular feature regardless of the headers of the system on
which it was built.

The second kind of version is the version of the system that OVS runs
on.  Unless a given feature is one that is supported by every version
that OVS cares about, OVS needs to test at runtime whether the feature
is supported and, if not, fall back to the older feature.  I don't see
that in this code.  Instead, it looks to me like it assumes that if the
feature was available at build time, then it is available at runtime.
This is not a good way to do things, since we want people to be able to
get builds from distributors such as Red Hat or Debian and then run
those builds on a diverse collection of kernels.

One specific comment I have here is that, in acinclude.m4, it would be
better to use AC_CHECK_TYPE or AC_CHECK_TYPES thatn OVS_GREP_IFELSE.
The latter is for testing for kernel builds only; we can't use the
normal AC_* tests for those because we often can't successfully build
kernel headers using the compiler and flags that Autoconf sets up for
building OVS.

Thanks,

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

Re: [ovs-dev] [PATCHv6] userspace: Add GTP-U support.

[Roni Bar Yanai]

Hi William,

Two comments, please inline.

>-Original Message-
>From: William Tu 
>Sent: Thursday, January 16, 2020 9:45 PM
>To: d...@openvswitch.org
>Cc: Roni Bar Yanai ; acon...@redhat.com;
>yangy...@inspur.com; fhal...@redhat.com; ashishvarma@gmail.com;
>b...@ovn.org
>Subject: [PATCHv6] userspace: Add GTP-U support.
>
>GTP, GPRS Tunneling Protocol, is a group of IP-based communications
>protocols used to carry general packet radio service (GPRS) within
>GSM, UMTS and LTE networks.  GTP protocol has two parts: Signalling
>(GTP-Control, GTP-C) and User data (GTP-User, GTP-U). GTP-C is used
>for setting up GTP-U protocol, which is an IP-in-UDP tunneling
>protocol. Usually GTP is used in connecting between base station for
>radio, Serving Gateway (S-GW), and PDN Gateway (P-GW).
>
>This patch implements GTP-U protocol for userspace datapath,
>supporting only required header fields and G-PDU message type.
>See spec in:
>https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftools.ietf.o
>rg%2Fhtml%2Fdraft-hmm-dmm-5g-uplane-analysis-
>00data=02%7C01%7Croniba%40mellanox.com%7Cf870ce16727e40fbf3cf08d
>79abcc4ef%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C6371480078905
>90483sdata=vU5nHP%2BTF1GqyHJfyfyYKhdla%2FT8BblYRCu2NUfxt7Q%3D
>reserved=0
>
>Signed-off-by: Yi Yang 
>Co-authored-by: Yi Yang 
>Signed-off-by: William Tu 
>---
>v5 -> v6:
>  - rebase to master
>  - travis:
>https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-
>ci.org%2Fwilliamtu%2Fovs-
>travis%2Fbuilds%2F638083655data=02%7C01%7Croniba%40mellanox.com%
>7Cf870ce16727e40fbf3cf08d79abcc4ef%7Ca652971c7d2e4d9ba6a4d149256f461b%7
>C0%7C0%7C637148007890590483sdata=lgdyvMiOvCjgEiLcFtqJvW2n2sm2clB
>aBqkwoWCzBbg%3Dreserved=0
>
>v4 -> v5:
>  - address Ben and Aaron comments
>1) flow_get_metadata, format_flow_tunnel
>2) use of ?: in MSVS
>3) tun_key_to_attr
>4) handling PT_GTPU_MSG packets
>5) make gtpu_flags and gtpu_msgtype read-only
>  - use be32_to_be64 and be64_to_be32
>  - make gtpu_flags as hexadicimal, gtpu_msgtype as decimal
>  - remove PT_GTPU_MSG
>  Address Roni's comments
>  - for non-GPDU msgtype, don't pop header
>  - add seq number flags parsing, push/pop header support
>  - refactor netdev_tnl_push_udp_header()
>
>v3 -> v4:
>  - applied Ben's doc revise
>  - increment FLOW_WC_SEQ to 42
>  - minor fixes
>  - travis:
>https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-
>ci.org%2Fwilliamtu%2Fovs-
>travis%2Fbuilds%2F621289678data=02%7C01%7Croniba%40mellanox.com%
>7Cf870ce16727e40fbf3cf08d79abcc4ef%7Ca652971c7d2e4d9ba6a4d149256f461b%7
>C0%7C0%7C637148007890590483sdata=6%2FPIBAFpCtpWXip89BU8B3R6S8B
>2CZFWRxB3rYNRp9Q%3Dreserved=0
>
>v2 -> v3:
>  - pick up the code from v2, rebase to master
>  - many fixes in code, docs, and add more tests
>  - travis:
>https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-
>ci.org%2Fwilliamtu%2Fovs-
>travis%2Fbuilds%2F619799361data=02%7C01%7Croniba%40mellanox.com%
>7Cf870ce16727e40fbf3cf08d79abcc4ef%7Ca652971c7d2e4d9ba6a4d149256f461b%7
>C0%7C0%7C637148007890590483sdata=ESUBrHITGL7Tn4PuyoCiWq0%2F%2
>FaxTDqIuckfw2ynrAyQ%3Dreserved=0
>
>v1 -> v2:
>  - Add new packet_type PT_GTPU_MSG to handle GTP-U signaling message,
>GTP-U signaling message should be steered into a control plane handler
>by explicit openflow entry in flow table.
>  - Fix gtpu_flags and gptu_msgtype set issue.
>  - Verify communication with kernel GTP implementation from Jiannan
>Ouyang.
>  - Fix unit test issue and make sure all the unit tests can pass.
>  - This patch series add GTP-U tunnel support in DPDK userspace,
>GTP-U is used for carrying user data within the GPRS core network and
>between the radio access network and the core network. The user data
>transported can be packets in any of IPv4, IPv6, or PPP formats.
>---
> Documentation/faq/configuration.rst   |  13 ++
> Documentation/faq/releases.rst|   1 +
> NEWS  |   3 +
> datapath/linux/compat/include/linux/openvswitch.h |   2 +
> include/openvswitch/flow.h|   4 +-
> include/openvswitch/match.h   |   6 +
> include/openvswitch/meta-flow.h   |  28 
> include/openvswitch/packets.h |   4 +-
> lib/dpif-netlink-rtnl.c   |   5 +
> lib/dpif-netlink.c|   5 +
> lib/flow.c|  28 ++--
> lib/flow.h|   2 +-
> lib/match.c   |  36 -
> lib/meta-flow.c   |  38 +
> lib/meta-flow.xml |  79 ++-
> lib/netdev-native-tnl.c   | 165 --
> lib/netdev-native-tnl.h   |  13 ++
> lib/netdev-vport.c   

Re: [ovs-dev] [PATCH] docs: Add header install command for afxdp.

[Ben Pfaff]

On Tue, Jan 21, 2020 at 09:48:28AM -0800, William Tu wrote:
> The 'XDP_RING_NEED_WAKEUP' and related flags are defined if_xdp.h, so after
> installing newer kernel, users have to update the kernel's header files,
> by doing:
>   $ make headers_install INSTALL_HDR_PATH=/usr
> 
> Otherwise the following error shows:
> /usr/local/include/bpf/xsk.h: In function 'xsk_ring_prod__needs_wakeup':
> /usr/local/include/bpf/xsk.h:82:21: error: 'XDP_RING_NEED_WAKEUP' undeclared \
>   (first use in this function)
>   return *r->flags & XDP_RING_NEED_WAKEUP;
> 
> Reported-by: Tomek Osinski 
> Reported-at: https://osinstom.github.io/en/tutorial/ovs-afxdp-installation/
> Signed-off-by: William Tu 
> ---
>  Documentation/intro/install/afxdp.rst | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/intro/install/afxdp.rst 
> b/Documentation/intro/install/afxdp.rst
> index c4685fa7ebac..2683d8301bb7 100644
> --- a/Documentation/intro/install/afxdp.rst
> +++ b/Documentation/intro/install/afxdp.rst
> @@ -125,6 +125,7 @@ Second, go into the Linux source directory and build 
> libbpf in the tools
>  directory::
>  
>cd bpf-next/
> +  make headers_install INSTALL_HDR_PATH=/usr
>cd tools/lib/bpf/
>make && make install
>make install_headers

This seems reasonable.

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

Re: [ovs-dev] [PATCHv6] userspace: Add GTP-U support.

[Ben Pfaff]

On Fri, Jan 17, 2020 at 04:24:42PM -0800, William Tu wrote:
> On Fri, Jan 17, 2020 at 02:14:27PM -0800, Ben Pfaff wrote:
> > On Thu, Jan 16, 2020 at 11:45:20AM -0800, William Tu wrote:
> > > GTP, GPRS Tunneling Protocol, is a group of IP-based communications
> > > protocols used to carry general packet radio service (GPRS) within
> > > GSM, UMTS and LTE networks.  GTP protocol has two parts: Signalling
> > > (GTP-Control, GTP-C) and User data (GTP-User, GTP-U). GTP-C is used
> > > for setting up GTP-U protocol, which is an IP-in-UDP tunneling
> > > protocol. Usually GTP is used in connecting between base station for
> > > radio, Serving Gateway (S-GW), and PDN Gateway (P-GW).
> > > 
> > > This patch implements GTP-U protocol for userspace datapath,
> > > supporting only required header fields and G-PDU message type.
> > > See spec in:
> > > https://tools.ietf.org/html/draft-hmm-dmm-5g-uplane-analysis-00
> > > 
> > > Signed-off-by: Yi Yang 
> > > Co-authored-by: Yi Yang 
> > > Signed-off-by: William Tu 
> > 
> > Thanks for the patch!
> > 
> > I have only a few comments.
> > 
> > To me, the "update" in the name netdev_tnl_update_udp_csum() suggests
> > that it's starting from an existing checksum and then adding a few more
> > things into it.  I think that it calculates the whole checksum from the
> > beginning.  I would say "calculate" or "calc" or "set" instead of
> > "update".
> > 
> > In netdev_gtpu_push_header(), I think that be32_to_be16(htonl(...)) is
> > the same as htons().
> > 
> > In ovs_parse_tnl_push(), I suggest using "%"SCNi8, etc., instead of
> > "0x%"SCNx8.  That allows the user to specify the values in decimal if
> > desired.  (That is particularly desirable if the value is 0, since it's
> > not really a normal thing to write 0x0 if one does not need to.)
> > 
> > Acked-by: Ben Pfaff 
> 
> Thanks Ben,
> I will fix the above and send v7.
> Probably wait for a couple days to see if there is any more feedbacks.

That makes sense to me, especially since neither of us really
understands GTP-U properly, even though we can read the specs.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 0/3] Add support for TSO with DPDK

[William Tu]

On Thu, Jan 16, 2020 at 9:01 AM Flavio Leitner  wrote:
>
> Abbreviated as TSO, TCP Segmentation Offload is a feature which enables
> the network stack to delegate the TCP segmentation to the NIC reducing
> the per packet CPU overhead.
>
> A guest using vhost-user interface with TSO enabled can send TCP packets
> much bigger than the MTU, which saves CPU cycles normally used to break
> the packets down to MTU size and to calculate checksums.
>
> It also saves CPU cycles used to parse multiple packets/headers during
> the packet processing inside virtual switch.
>
> If the destination of the packet is another guest in the same host, then
> the same big packet can be sent through a vhost-user interface skipping
> the segmentation completely. However, if the destination is not local,
> the NIC hardware is instructed to do the TCP segmentation and checksum
> calculation.
>
> The first 2 patches are not really part of TSO support, but they are
> required to make sure everything works.
>
> There are good improvements sending to or receiving from veth pairs or
> tap devices as well. See the iperf3 results below:
>
> [*] veth with ethtool tx off.
>

Hi Flavio,

I want to test performance of namespace to namespace using veth, hoping to
see TSO packets. Using below setup:
  iperf -c (ns0) -> veth peer -> OVS -> veth peer -> iperf -s (ns1)

With current master I'm not able to see large packet size being sent.
I compile ovs with --with-dpdk and,
$ ovs-vsctl set Open_vSwitch . other_config:userspace-tso-enable=true

At ns0 and ns1, enable tso by ethtool sg and tso on
The veth driver shows
# ip netns exec at_ns0 ethtool -k p0
Features for p0:
Cannot get device udp-fragmentation-offload settings: Operation not supported
rx-checksumming: on
tx-checksumming: off
tx-checksum-ipv4: off [fixed]
tx-checksum-ip-generic: off
tx-checksum-ipv6: off [fixed]
tx-checksum-fcoe-crc: off [fixed]
tx-checksum-sctp: off
scatter-gather: on
tx-scatter-gather: on
tx-scatter-gather-fraglist: on
tcp-segmentation-offload: off
tx-tcp-segmentation: off [requested on]
tx-tcp-ecn-segmentation: off [requested on]
tx-tcp-mangleid-segmentation: off [requested on]
   tx-tcp6-segmentation: off [requested on]

But I'm still seeing 1500 packet size, and about 1.3Gbps performance.
Is there anything I'm missing?

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

Re: [ovs-dev] [patch] datapath-windows: Append tunnel info to upcall for correct template

[Ben Pfaff]

I expect that, of the OVS maintainers, Alin is most qualified to
review it.  Alin, are you planning to take a look at it?

On Tue, Jan 21, 2020 at 02:52:54AM +, Amber Hu via dev wrote:
> Hi,
> 
> May I ask the process of reviewing for the below patch 
> https://patchwork.ozlabs.org/patch/1222463/
> Thanks for your time.
> 
> BR/Amber
> 
> From: Amber Hu 
> Date: Tuesday, January 14, 2020 at 13:49
> To: "d...@openvswitch.org" , Alin Serdean 
> 
> Cc: Qiong Wang , Wenyu Zhang , Jinjun 
> Gao 
> Subject: [patch] datapath-windows: Append tunnel info to upcall for correct 
> template
> 
> Formerly, there is no tunnel information appended in the upcall’s
> packet data, which is expected by IPFIX in userspace to calculate
> the template for exporting the sampled flow record of on egress
> tunnel port.
> To fix this, during performing OvsOutputUserspaceAction(), we
> would check whether it is initiated by the sampling on egress
> tunnel which would be indicated by the attribute as
> OVS_USERSPACE_ATTR_EGRESS_TUN_PORT in the nested attribute
> list. If so, we would append the tunKey in OvsForwardingContext
> indexed by OVS_PACKET_ATTR_EGRESS_TUN_KEY to the upcall.
> Besides, at this point, the source transport port and  source ip
> address are not available in the structure, so we have to fill it in the
> way how the packet would be capsulated during performing
> OvsEncapGeneve(), which is following the
> OvsOutputUserspaceAction() unfortunately.
> I have tested the IPFIX functionality with the change, we could see the
> template is correct and the expected tunnel information could be
> packed in the IPFIX packet finally. The traffic for test is generated by
> PING utility.
> 
> Issue: 2449045
> Reported-by: Meng Yang mailto:ym...@vmware.com>>
> Reported-at: https://bugzilla.eng.vmware.com/show_bug.cgi?id=2449045
> Signed-off-by: Amber Hu 
> 
> From 8262494fb9a353c3adbe02cfc4c932231c34b3ed Mon Sep 17 00:00:00 2001
> From: Amber Hu 
> Date: Sun, 12 Jan 2020 22:40:05 -0800
> Subject: [PATCH] datapath-windows: Append tunnel info to upcall for correct
> template
> 
> Issue: #2449045
> Signed-off-by: Amber Hu 
> ---
> datapath-windows/ovsext/Actions.c | 43 ++-
> datapath-windows/ovsext/Flow.c| 12 +++
> datapath-windows/ovsext/User.c| 10 +++--
> datapath-windows/ovsext/User.h|  1 +
> 4 files changed, 59 insertions(+), 7 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/Actions.c 
> b/datapath-windows/ovsext/Actions.c
> index 0c4d64b..936e68b 100644
> --- a/datapath-windows/ovsext/Actions.c
> +++ b/datapath-windows/ovsext/Actions.c
> @@ -1839,11 +1839,16 @@ OvsOutputUserspaceAction(OvsForwardingContext 
> *ovsFwdCtx,
>   const PNL_ATTR attr)
> {
>  NTSTATUS status = NDIS_STATUS_SUCCESS;
> -PNL_ATTR userdataAttr;
> -PNL_ATTR queueAttr;
> +PNL_ATTR userdataAttr = NULL;
> +PNL_ATTR queueAttr = NULL;
> +PNL_ATTR egrTunAttr = NULL;
> +PNL_ATTR a = NULL;
>  POVS_PACKET_QUEUE_ELEM elem;
>  POVS_PACKET_HDR_INFO layers = >layers;
>  BOOLEAN isRecv = FALSE;
> +INT rem = 0;
> +OVS_FWD_INFO fwdInfo;
> +OvsIPv4TunnelKey tunKey;
>  POVS_VPORT_ENTRY vport = OvsFindVportByPortNo(ovsFwdCtx->switchContext,
>ovsFwdCtx->srcVportNo);
> @@ -1855,13 +1860,41 @@ OvsOutputUserspaceAction(OvsForwardingContext 
> *ovsFwdCtx,
>  }
>  }
> -queueAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_PID);
> -userdataAttr = NlAttrFindNested(attr, OVS_USERSPACE_ATTR_USERDATA);
> +NL_ATTR_FOR_EACH_UNSAFE(a, rem, NlAttrData(attr), NlAttrGetSize(attr)) {
> +switch (NlAttrType(a)) {
> +case OVS_USERSPACE_ATTR_PID:
> +queueAttr = a;
> +break;
> +case OVS_USERSPACE_ATTR_USERDATA:
> +userdataAttr = a;
> +break;
> +case OVS_USERSPACE_ATTR_EGRESS_TUN_PORT:
> +/* Indicate the packet is from egress tunnel port */
> +egrTunAttr = a;
> +break;
> +default:
> +break;
> +}
> +}
> +
> +/* Fill tunnel key to export to usersspace to calculate the template id 
> */
> +if (egrTunAttr) {
> +RtlCopyMemory(, >tunKey, sizeof tunKey);
> +if (!tunKey.src) {
> +status = OvsLookupIPFwdInfo(tunKey.src, tunKey.dst, );
> +if (status == NDIS_STATUS_SUCCESS && tunKey.dst == 
> fwdInfo.dstIpAddr) {
> +tunKey.src = fwdInfo.srcIpAddr;
> +}
> +}
> +tunKey.flow_hash = tunKey.flow_hash?tunKey.flow_hash:MAXINT16;
> +}
>  elem = OvsCreateQueueNlPacket(NlAttrData(userdataAttr),
>NlAttrGetSize(userdataAttr),
>OVS_PACKET_CMD_ACTION,
> -  vport, key, ovsFwdCtx->curNbl,
> +   

[ovs-dev] [PATCH] docs: Add header install command for afxdp.

[William Tu]

The 'XDP_RING_NEED_WAKEUP' and related flags are defined if_xdp.h, so after
installing newer kernel, users have to update the kernel's header files,
by doing:
  $ make headers_install INSTALL_HDR_PATH=/usr

Otherwise the following error shows:
/usr/local/include/bpf/xsk.h: In function 'xsk_ring_prod__needs_wakeup':
/usr/local/include/bpf/xsk.h:82:21: error: 'XDP_RING_NEED_WAKEUP' undeclared \
  (first use in this function)
  return *r->flags & XDP_RING_NEED_WAKEUP;

Reported-by: Tomek Osinski 
Reported-at: https://osinstom.github.io/en/tutorial/ovs-afxdp-installation/
Signed-off-by: William Tu 
---
 Documentation/intro/install/afxdp.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/intro/install/afxdp.rst 
b/Documentation/intro/install/afxdp.rst
index c4685fa7ebac..2683d8301bb7 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -125,6 +125,7 @@ Second, go into the Linux source directory and build libbpf 
in the tools
 directory::
 
   cd bpf-next/
+  make headers_install INSTALL_HDR_PATH=/usr
   cd tools/lib/bpf/
   make && make install
   make install_headers
-- 
2.7.4

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

[ovs-dev] [PATCHv8] userspace: Add GTP-U support.

[William Tu]

GTP, GPRS Tunneling Protocol, is a group of IP-based communications
protocols used to carry general packet radio service (GPRS) within
GSM, UMTS and LTE networks.  GTP protocol has two parts: Signalling
(GTP-Control, GTP-C) and User data (GTP-User, GTP-U). GTP-C is used
for setting up GTP-U protocol, which is an IP-in-UDP tunneling
protocol. Usually GTP is used in connecting between base station for
radio, Serving Gateway (S-GW), and PDN Gateway (P-GW).

This patch implements GTP-U protocol for userspace datapath,
supporting only required header fields and G-PDU message type.
See spec in:
https://tools.ietf.org/html/draft-hmm-dmm-5g-uplane-analysis-00

Signed-off-by: Feng Yang 
Co-authored-by: Feng Yang 
Signed-off-by: Yi Yang 
Co-authored-by: Yi Yang 
Signed-off-by: William Tu 
Acked-by: Ben Pfaff 
---
v7 -> v8:
  - Add Feng Yang as co-authored
  - fix checkpatch error
  - https://travis-ci.org/williamtu/ovs-travis/builds/640009759

v6 -> v7:
  - address Ben's feedback
- function name: use netdev_tnl_calc_udp_csum
- remove unnecessary be32_to_be16(htonl
- use SCNi8 instead of SCNx8
- Travis: https://travis-ci.org/williamtu/ovs-travis/builds/638683932

v5 -> v6:
  - rebase to master
  - travis: https://travis-ci.org/williamtu/ovs-travis/builds/638083655

v4 -> v5:
  - address Ben and Aaron comments
1) flow_get_metadata, format_flow_tunnel
2) use of ?: in MSVS
3) tun_key_to_attr
4) handling PT_GTPU_MSG packets
5) make gtpu_flags and gtpu_msgtype read-only
  - use be32_to_be64 and be64_to_be32
  - make gtpu_flags as hexadicimal, gtpu_msgtype as decimal
  - remove PT_GTPU_MSG
  Address Roni's comments
  - for non-GPDU msgtype, don't pop header
  - add seq number flags parsing, push/pop header support
  - refactor netdev_tnl_push_udp_header()

v3 -> v4:
  - applied Ben's doc revise
  - increment FLOW_WC_SEQ to 42
  - minor fixes
  - travis: https://travis-ci.org/williamtu/ovs-travis/builds/621289678

v2 -> v3:
  - pick up the code from v2, rebase to master
  - many fixes in code, docs, and add more tests
  - travis: https://travis-ci.org/williamtu/ovs-travis/builds/619799361

v1 -> v2:
  - Add new packet_type PT_GTPU_MSG to handle GTP-U signaling message,
GTP-U signaling message should be steered into a control plane handler
by explicit openflow entry in flow table.
  - Fix gtpu_flags and gptu_msgtype set issue.
  - Verify communication with kernel GTP implementation from Jiannan
Ouyang.
  - Fix unit test issue and make sure all the unit tests can pass.
  - This patch series add GTP-U tunnel support in DPDK userspace,
GTP-U is used for carrying user data within the GPRS core network and
between the radio access network and the core network. The user data
transported can be packets in any of IPv4, IPv6, or PPP formats.
---
 Documentation/faq/configuration.rst   |  13 ++
 Documentation/faq/releases.rst|   1 +
 NEWS  |   3 +
 datapath/linux/compat/include/linux/openvswitch.h |   2 +
 include/openvswitch/flow.h|   4 +-
 include/openvswitch/match.h   |   6 +
 include/openvswitch/meta-flow.h   |  28 
 include/openvswitch/packets.h |   4 +-
 lib/dpif-netlink-rtnl.c   |   5 +
 lib/dpif-netlink.c|   5 +
 lib/flow.c|  28 ++--
 lib/flow.h|   2 +-
 lib/match.c   |  36 -
 lib/meta-flow.c   |  38 +
 lib/meta-flow.xml |  79 ++-
 lib/netdev-native-tnl.c   | 165 --
 lib/netdev-native-tnl.h   |  13 ++
 lib/netdev-vport.c|  25 +++-
 lib/nx-match.c|   8 +-
 lib/odp-util.c| 123 +++-
 lib/odp-util.h|   2 +-
 lib/ofp-match.c   |   2 +-
 lib/packets.h |  68 +
 lib/tnl-ports.c   |   3 +
 ofproto/ofproto-dpif-rid.h|   2 +-
 ofproto/ofproto-dpif-xlate.c  |   3 +-
 tests/ofproto.at  |   2 +-
 tests/tunnel-push-pop.at  |  22 +++
 tests/tunnel.at   |  76 ++
 vswitchd/vswitch.xml  |  24 
 30 files changed, 752 insertions(+), 40 deletions(-)

diff --git a/Documentation/faq/configuration.rst 
b/Documentation/faq/configuration.rst
index ff3b71a5d4ef..4a98740c5d4d 100644
--- a/Documentation/faq/configuration.rst
+++ b/Documentation/faq/configuration.rst
@@ -225,6 +225,19 @@ Q: Does Open 

[ovs-dev] Es momento de darle un giro a tu empresa

[Whatsapp Business]

Miércoles 12 de Febrero | Horario de 10:00 a 14:00 hrs.  |  (hora del centro de 
México) 

- ¿Cómo aumentar tus ventas con Whatsapp Business? - 

¿De qué hablaremos?

Aprende a vender por la aplicación más usada del mundo utilizando herramientas 
digitales que aumentarán tu conversión actual de leads.
Es el momento de darle un giro a tu negocio o empresa, crea contenido, diseños 
y videos profesionales para enamorar a tus prospectos.
¡Haz que tus ejecutivos de venta VENDAN por WhatsApp fuera de su horario 
laboral!


¿Qué aprenderás?:


- WhatsApp cómo poderosa herramienta de ventas.
- Errores comunes al utilizar WhatsApp cómo herramienta de marketing.
- Atención al cliente como estrategia de fidelización.
- Robot en Whatsapp Business

Solicita información respondiendo a este correo con la palabra WhatsApp junto 
con los siguientes datos:

Nombre:
Correo electrónico:
Número telefónico:

Dirigido a: Gerente y ejecutivos de ventas. Emprendedores, comerciantes y todo 
aquel que quiera conocer herramientas 
tecnológicas para impulsar sus ventas.

Números de Atención: 

(045) 55 15 54 66 30 - (045) 55 85567293 - (045) 5530167085 

En caso de que haya recibido este correo sin haberlo solicitado o si desea 
dejar de recibir nuestra promoción favor de responder
con la palabra baja o enviar un correo a bajas@ innovalearn.net


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

Re: [ovs-dev] [PATCH v3] Documentation: add notes for TSO & i40e

[Stokes, Ian]

On 1/20/2020 2:45 PM, Flavio Leitner wrote:



ACK, thanks!
fbl



Thanks All,

pushed to master.

Regards
Ian


On Mon, Jan 20, 2020 at 01:09:36PM +, Ciara Loftus wrote:

When using TSO in OVS-DPDK with an i40e device, the following
patch is required for DPDK, which fixes an issue on the TSO path:
https://patches.dpdk.org/patch/64136/
Document this as a limitation until a DPDK release with the fix
included is supported by OVS.

Also, document best known methods for performance tuning when
testing TSO with the tool iperf.

Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support")
Signed-off-by: Ciara Loftus 
Acked-by: Flavio Leitner 
---
v3:
- Added Fixes tag
- Removed unwanted manpages.mk change

v2:
- rebased to master
- changed patch links from net-next tree to patchwork
---
  Documentation/topics/userspace-tso.rst | 27 ++
  1 file changed, 27 insertions(+)

diff --git a/Documentation/topics/userspace-tso.rst 
b/Documentation/topics/userspace-tso.rst
index 893c64839..94eddc0b2 100644
--- a/Documentation/topics/userspace-tso.rst
+++ b/Documentation/topics/userspace-tso.rst
@@ -96,3 +96,30 @@ datapath must support TSO or packets using that feature will 
be dropped
  on ports without TSO support.  That also means guests using vhost-user
  in client mode will receive TSO packet regardless of TSO being enabled
  or disabled within the guest.
+
+When the NIC performing the segmentation is using the i40e DPDK PMD, a fix
+must be included in the DPDK build, otherwise TSO will not work. The fix can
+be found on `DPDK patchwork`__.
+
+__ https://patches.dpdk.org/patch/64136/
+
+This fix is expected to be included in the 19.11.1 release. When OVS migrates
+to this DPDK release, this limitation can be removed.
+
+~~
+Performance Tuning
+~~
+
+iperf is often used to test TSO performance. Care needs to be taken when
+configuring the environment in which the iperf server process is being run.
+Since the iperf server uses the NIC's kernel driver, IRQs will be generated.
+By default with some NICs eg. i40e, the IRQs will land on the same core as that
+which is being used by the server process, provided the number of NIC queues is
+greater or equal to that lcoreid. This causes contention between the iperf
+server process and the IRQs. For optimal performance, it is suggested to pin
+the IRQs to their own core. To change the affinity associated with a given IRQ
+number, you can 'echo' the desired coremask to the file
+/proc/irq//smp_affinity
+For more on SMP affinity, refer to the `Linux kernel documentation`__.
+
+__ https://www.kernel.org/doc/Documentation/IRQ-affinity.txt
--
2.17.1

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



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

Re: [ovs-dev] [PATCHv7] userspace: Add GTP-U support.

[William Tu]

On Fri, Jan 17, 2020 at 7:00 PM 0-day Robot  wrote:
>
> Bleep bloop.  Greetings William Tu, I am a robot and I have tried out your 
> patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> ERROR: C99 style comment
> #816 FILE: lib/netdev-native-tnl.c:808:
> //*seqno = be32_to_be16(htonl(tnl_cfg->seqno++));
>
> Lines checked: 1571, Warnings: 0, Errors: 1
>
>
> Please check this out.  If you feel there has been an error, please email 
> acon...@redhat.com
>
> Thanks,
> 0-day Robot

Thanks, I will fix it this mistake.
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] tc: handle packet mark of zero

[Simon Horman]

On Thu, Jan 16, 2020 at 09:23:11AM -0500, Aaron Conole wrote:
> Simon Horman  writes:
> 
> > On Thu, Jan 16, 2020 at 04:59:54AM -0500, 0-day Robot wrote:
> >> Bleep bloop.  Greetings Simon Horman, I am a robot and I have tried out 
> >> your patch.
> >> Thanks for your contribution.
> >> 
> >> I encountered some error that I wasn't expecting.  See the details below.
> >> 
> >> 
> >> checkpatch:
> >> WARNING: Unexpected sign-offs from developers who are not authors or
> >> co-authors or committers: Simon Horman 
> >> Lines checked: 44, Warnings: 1, Errors: 0
> >
> > Is the correct tag Co-Authored-by rather than Co-Authored as used in
> > this patch?
> 
> Yes.
> 
> Co-authored-by
> 
> Not sure if we should make the tags a bit more lenient, but that's a
> separate discussion. :)

On Thu, Jan 16, 2020 at 11:35:54AM -0800, Ben Pfaff wrote:
> On Thu, Jan 16, 2020 at 11:20:20AM +0100, Simon Horman wrote:
> > On Thu, Jan 16, 2020 at 04:59:54AM -0500, 0-day Robot wrote:
> > > Bleep bloop.  Greetings Simon Horman, I am a robot and I have tried out 
> > > your patch.
> > > Thanks for your contribution.
> > > 
> > > I encountered some error that I wasn't expecting.  See the details below.
> > > 
> > > 
> > > checkpatch:
> > > WARNING: Unexpected sign-offs from developers who are not authors or 
> > > co-authors or committers: Simon Horman 
> > > Lines checked: 44, Warnings: 1, Errors: 0
> > 
> > Is the correct tag Co-Authored-by rather than Co-Authored as used in
> > this patch?
> 
> Yes, it should be Co-authored-by.  See
> Documentation/internals/contributing/submitting-patches.rst

Thanks. Fixing up the tag seems easy enough.  But I'd appreciate some
feedback on if people are happy with me applying this change or not.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2 1/2] expr: Evaluate the condition expression in a separate step.

[numans]

From: Numan Siddique 

A new function is added - expr_evaluate_condition() which evaluates
the condition expression - is_chassis_resident. expr_simply() will
no longer evaluates this condition.

An upcoming commit needs this in order to cache the logical flow expressions
so that every lflow_*() function which calls consider_logical_flow() doesn't
need to convert the logical flow match to an expression. Instead the expr tree
for the logical flow is cached. Since we can't cache the is_chassis_resident
condition, it is separated out.

Acked-by: Mark Michelson 
Acked-by: Han Zhou 
Signed-off-by: Numan Siddique 
---
 controller/lflow.c|  3 ++-
 include/ovn/expr.h| 10 
 lib/expr.c| 55 +--
 tests/test-ovn.c  | 10 
 utilities/ovn-trace.c |  5 +++-
 5 files changed, 60 insertions(+), 23 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index a6893201e..93ec53a1c 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -661,8 +661,9 @@ consider_logical_flow(
 .chassis = chassis,
 .active_tunnels = active_tunnels,
 };
-expr = expr_simplify(expr, is_chassis_resident_cb, _aux);
+expr = expr_simplify(expr);
 expr = expr_normalize(expr);
+expr = expr_evaluate_condition(expr, is_chassis_resident_cb, _aux);
 uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, ,
);
 expr_destroy(expr);
diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index 21bf51c22..00a021236 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -404,10 +404,12 @@ void expr_destroy(struct expr *);
 
 struct expr *expr_annotate(struct expr *, const struct shash *symtab,
char **errorp);
-struct expr *expr_simplify(struct expr *,
-   bool (*is_chassis_resident)(const void *c_aux,
-   const char *port_name),
-   const void *c_aux);
+struct expr *expr_simplify(struct expr *);
+struct expr *expr_evaluate_condition(
+struct expr *,
+bool (*is_chassis_resident)(const void *c_aux,
+const char *port_name),
+const void *c_aux);
 struct expr *expr_normalize(struct expr *);
 
 bool expr_honors_invariants(const struct expr *);
diff --git a/lib/expr.c b/lib/expr.c
index e5e4d21b7..12557e3ca 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -2033,10 +2033,10 @@ expr_simplify_relational(struct expr *expr)
 
 /* Resolves condition and replaces the expression with a boolean. */
 static struct expr *
-expr_simplify_condition(struct expr *expr,
-bool (*is_chassis_resident)(const void *c_aux,
+expr_evaluate_condition__(struct expr *expr,
+  bool (*is_chassis_resident)(const void *c_aux,
 const char *port_name),
-const void *c_aux)
+  const void *c_aux)
 {
 bool result;
 
@@ -2054,13 +2054,41 @@ expr_simplify_condition(struct expr *expr,
 return expr_create_boolean(result);
 }
 
+struct expr *
+expr_evaluate_condition(struct expr *expr,
+bool (*is_chassis_resident)(const void *c_aux,
+const char *port_name),
+const void *c_aux)
+{
+struct expr *sub, *next;
+
+switch (expr->type) {
+case EXPR_T_AND:
+case EXPR_T_OR:
+ LIST_FOR_EACH_SAFE (sub, next, node, >andor) {
+ovs_list_remove(>node);
+struct expr *e = expr_evaluate_condition(sub, is_chassis_resident,
+ c_aux);
+e = expr_fix(e);
+expr_insert_andor(expr, next, e);
+}
+return expr_fix(expr);
+
+case EXPR_T_CONDITION:
+return expr_evaluate_condition__(expr, is_chassis_resident, c_aux);
+
+case EXPR_T_CMP:
+case EXPR_T_BOOLEAN:
+return expr;
+}
+
+OVS_NOT_REACHED();
+}
+
 /* Takes ownership of 'expr' and returns an equivalent expression whose
  * EXPR_T_CMP nodes use only tests for equality (EXPR_R_EQ). */
 struct expr *
-expr_simplify(struct expr *expr,
-  bool (*is_chassis_resident)(const void *c_aux,
-  const char *port_name),
-  const void *c_aux)
+expr_simplify(struct expr *expr)
 {
 struct expr *sub, *next;
 
@@ -2075,8 +2103,7 @@ expr_simplify(struct expr *expr,
 case EXPR_T_OR:
 LIST_FOR_EACH_SAFE (sub, next, node, >andor) {
 ovs_list_remove(>node);
-expr_insert_andor(expr, next,
-  expr_simplify(sub, is_chassis_resident, c_aux));
+expr_insert_andor(expr, next, expr_simplify(sub));
 }
 return expr_fix(expr);
 
@@ -2084,7 +2111,7 @@ expr_simplify(struct expr *expr,
 return 

[ovs-dev] [PATCH ovn v2 2/2] ovn-controller: Cache logical flow expr tree for each lflow.

[numans]

From: Numan Siddique 

This patch caches the logical flow expr tree for each logical flow. This
cache is stored as an hashmap in the output_flow engine data. When a logical
flow is deleted, the expr tree for that lflow is deleted in the
flow_output_sb_logical_flow_handler().

Below are the details of the OVN resource in my setup

No of logical switches - 49
No of logical ports- 1191
No of logical routers  - 7
No of logical ports- 51
No of ACLs - 1221
No of Logical flows- 664736

On a chassis hosting 7 distributed router ports and around 1090
port bindings, the no of OVS rules was 921162.

Without this patch, the function add_logical_flows() took ~15 seconds.
And with this patch it took ~10 seconds. There is a good 34% improvement
in logical flow processing.

Acked-by: Mark Michelson 
Signed-off-by: Numan Siddique 
---
 controller/lflow.c  | 192 ++--
 controller/lflow.h  |   9 +-
 controller/ovn-controller.c |  16 ++-
 3 files changed, 160 insertions(+), 57 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 93ec53a1c..997c59662 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -75,11 +75,13 @@ static bool consider_logical_flow(
 const struct shash *port_groups,
 const struct sset *active_tunnels,
 const struct sset *local_lport_ids,
+bool delete_expr_from_cache,
 struct ovn_desired_flow_table *,
 struct ovn_extend_table *group_table,
 struct ovn_extend_table *meter_table,
 struct lflow_resource_ref *lfrr,
-uint32_t *conj_id_ofs);
+uint32_t *conj_id_ofs,
+struct hmap *lflow_expr_cache);
 
 static bool
 lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp)
@@ -255,6 +257,66 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref 
*lfrr,
 free(lfrn);
 }
 
+/* Represents expr tree for the lflow. We don't store the
+ * match in this structure because -
+ *   - Whenever a flow match or action needs to be modified,
+ * ovn-northd deletes the existing flow in the logical_flow
+ * table and adds a new one.
+ *  We may want to revisit this and store match incase the behavior
+ *  of ovn-northd changes.
+ */
+struct lflow_expr {
+struct hmap_node node;
+struct uuid lflow_uuid; /* key */
+struct expr *expr;
+};
+
+static void
+lflow_expr_add(struct hmap *lflow_expr_cache,
+   const struct sbrec_logical_flow *lflow,
+   struct expr *lflow_expr)
+{
+struct lflow_expr *le = xmalloc(sizeof *le);
+le->lflow_uuid = lflow->header_.uuid;
+le->expr = lflow_expr;
+hmap_insert(lflow_expr_cache, >node, uuid_hash(>lflow_uuid));
+}
+
+static struct lflow_expr *
+lflow_expr_get(struct hmap *lflow_expr_cache,
+   const struct sbrec_logical_flow *lflow)
+{
+struct lflow_expr *le;
+size_t hash = uuid_hash(>header_.uuid);
+HMAP_FOR_EACH_WITH_HASH (le, node, hash, lflow_expr_cache) {
+if (uuid_equals(>lflow_uuid, >header_.uuid)) {
+return le;
+}
+}
+
+return NULL;
+}
+
+static void
+lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
+{
+hmap_remove(lflow_expr_cache, >node);
+expr_destroy(le->expr);
+free(le);
+}
+
+void
+lflow_expr_destroy(struct hmap *lflow_expr_cache)
+{
+struct lflow_expr *le;
+HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
+expr_destroy(le->expr);
+free(le);
+}
+
+hmap_destroy(lflow_expr_cache);
+}
+
 /* Adds the logical flows from the Logical_Flow table to flow tables. */
 static void
 add_logical_flows(
@@ -273,7 +335,8 @@ add_logical_flows(
 struct ovn_extend_table *group_table,
 struct ovn_extend_table *meter_table,
 struct lflow_resource_ref *lfrr,
-uint32_t *conj_id_ofs)
+uint32_t *conj_id_ofs,
+struct hmap *lflow_expr_cache)
 {
 const struct sbrec_logical_flow *lflow;
 
@@ -306,9 +369,9 @@ add_logical_flows(
chassis, _opts, _opts,
_ra_opts, _event_opts,
addr_sets, port_groups,
-   active_tunnels, local_lport_ids,
+   active_tunnels, local_lport_ids, false,
flow_table, group_table, meter_table,
-   lfrr, conj_id_ofs)) {
+   lfrr, conj_id_ofs, lflow_expr_cache)) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
 VLOG_ERR_RL(, "Conjunction id overflow when processing lflow "
 UUID_FMT, UUID_ARGS(>header_.uuid));
@@ -338,7 +401,8 @@ lflow_handle_changed_flows(
 struct ovn_extend_table *group_table,
 struct ovn_extend_table *meter_table,
 struct lflow_resource_ref *lfrr,
-uint32_t *conj_id_ofs)
+uint32_t *conj_id_ofs,
+struct hmap *lflow_expr_cache)
 {
 bool ret = true;

[ovs-dev] [PATCH ovn v2 0/2] Caching logical flow expr tree for each lflow

[numans]

From: Numan Siddique 

This patch series tries to improve the time taken to proceess logical
flows by caching the expr tree. For large scale deployments with lots
of logical flows, the logical flow processing to OpenFlow rules
takes a lot of time as it is CPU intensive.

This patch series tries to improve this by caching the expr tree
generated for each logical flow so that we don't have to generate the
expr tree for each lflow_run().

Below are the details of the OVN resource in my setup

No of logical switches - 49
No of logical ports- 1191
No of logical routers  - 7
No of logical ports- 51
No of ACLs - 1221
No of Logical flows- 664736

On a chassis hosting 7 distributed router ports and around 1090
port bindings, the no of OVS rules was 921162.

Without this patch, the function add_logical_flows() took ~15 seconds.
And with this patch it took ~10 seconds. There is a good 34% improvement
in logical flow processing.

v1 -> v2
===
 * Addressed review comments from Han.

Numan Siddique (2):
  expr: Evaluate the condition expression in a separate step.
  ovn-controller: Cache logical flow expr tree for each lflow.

 controller/lflow.c  | 191 +++-
 controller/lflow.h  |   9 +-
 controller/ovn-controller.c |  16 ++-
 include/ovn/expr.h  |  10 +-
 lib/expr.c  |  55 ---
 tests/test-ovn.c|  10 +-
 utilities/ovn-trace.c   |   5 +-
 7 files changed, 218 insertions(+), 78 deletions(-)

-- 
2.24.1

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

Re: [ovs-dev] [PATCH ovn 2/2] ovn-controller: Cache logical flow expr tree for each lflow.

[Numan Siddique]

On Tue, Jan 21, 2020 at 2:03 PM Han Zhou  wrote:
>
> On Thu, Jan 9, 2020 at 9:37 AM  wrote:
> >
> > From: Numan Siddique 
> >
> > This patch caches the logical flow expr tree for each logical flow. This
> > cache is stored as an hashmap in the output_flow engine data. When a
> logical
> > flow is deleted, the expr tree for that lflow is deleted in the
> > flow_output_sb_logical_flow_handler().
> >
> > Below are the details of the OVN resource in my setup
> >
> > No of logical switches - 49
> > No of logical ports- 1191
> > No of logical routers  - 7
> > No of logical ports- 51
> > No of ACLs - 1221
> > No of Logical flows- 664736
> >
> > On a chassis hosting 7 distributed router ports and around 1090
> > port bindings, the no of OVS rules was 921162.
> >
> > Without this patch, the function add_logical_flows() took ~15 seconds.
> > And with this patch it took ~10 seconds. There is a good 34% improvement
> > in logical flow processing.
>
> Great improvement! Thanks Numan. Please see my comments below.
>

Thanks for the review. v3 is on its way. PSB for few comments.

> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  controller/lflow.c  | 182 ++--
> >  controller/lflow.h  |   9 +-
> >  controller/ovn-controller.c |  17 +++-
> >  3 files changed, 154 insertions(+), 54 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 93ec53a1c..be46f0661 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -79,7 +79,9 @@ static bool consider_logical_flow(
> >  struct ovn_extend_table *group_table,
> >  struct ovn_extend_table *meter_table,
> >  struct lflow_resource_ref *lfrr,
> > -uint32_t *conj_id_ofs);
> > +uint32_t *conj_id_ofs,
> > +struct hmap *lflow_expr_cache,
> > +bool delete_expr_from_cache);
>
> It was a convention that output args are at the end of the list (Ben took a
> lot of effort to make it this way when we were implementing incremental
> processing), so it would be better to move the last one
> "delete_expr_from_cache" to the position before ovn_desired_flow_table.

Ack.


>
> >
> >  static bool
> >  lookup_port_cb(const void *aux_, const char *port_name, unsigned int
> *portp)
> > @@ -255,6 +257,59 @@ lflow_resource_destroy_lflow(struct
> lflow_resource_ref *lfrr,
> >  free(lfrn);
> >  }
> >
> > +struct lflow_expr {
> > +struct hmap_node node;
> > +struct uuid lflow_uuid; /* key */
> > +struct expr *expr;
> > +char *lflow_match;
> > +};
> > +
> > +static void
> > +lflow_expr_add(struct hmap *lflow_expr_cache,
> > +   const struct sbrec_logical_flow *lflow,
> > +   struct expr *lflow_expr)
> > +{
> > +struct lflow_expr *le = xmalloc(sizeof *le);
> > +le->lflow_uuid = lflow->header_.uuid;
> > +le->expr = lflow_expr;
> > +le->lflow_match = xstrdup(lflow->match);
> > +hmap_insert(lflow_expr_cache, >node, uuid_hash(>lflow_uuid));
> > +}
> > +
> > +static struct lflow_expr *
> > +lflow_expr_get(struct hmap *lflow_expr_cache,
> > +   const struct sbrec_logical_flow *lflow)
> > +{
> > +struct lflow_expr *le;
> > +size_t hash = uuid_hash(>header_.uuid);
> > +HMAP_FOR_EACH_WITH_HASH (le, node, hash, lflow_expr_cache) {
> > +if (!strcmp(lflow->match, le->lflow_match)) {
>
> I think here we should compare lflow uuid first to make sure we found the
> original lflow, and then compare lflow_match to make sure the match didn't
> change. If the lflow uuid matched but match string changed, it means the
> cache is no longer valid and we should delete it from the cache.
> Otherwise, if a lflow in SB is updated on the match field, it will end up
> with unused entries in the cache forever, unless the same lflow's match
> condition changed back.
> In fact, the case that a lflow's match condition changes may never happen
> at all considering northd's logic (correct me if I am wrong), and if this
> is true, then I don't think we need to compare the lflow->match at all, and
> then we don't need the lflow_match in the struct lflow_expr.
> In either case, it seems this code need some change.

I agree with you. Infact, I think matching the lflow uuid is just
sufficient. I confirmed the
ovn-northd logic. When ovn-northd computes a flow and this flow is
changed a bit (like match condition)
from the  flow in the SB DB logical_flow table, ovn-northd deletes the
existing flow in SB DB and adds
a new one.

https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L9471
https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L3718


>
> > +return le;
> > +}
> > +}
> > +
> > +return NULL;
> > +}
> > +
> > +static void
> > +lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
> > +{
> > +hmap_remove(lflow_expr_cache, >node);
> > +free(le->lflow_match);
> > +expr_destroy(le->expr);
> > +free(le);
> > +}
> > +
> > +void
> > 

Re: [ovs-dev] OVN Soft Freeze

[Dumitru Ceara]

On Tue, Jan 21, 2020 at 12:09 PM Mark Michelson  wrote:
>
> Hi all,
>
> OVN has entered its "soft freeze" state. That means that any new
> features that should be added prior to the next release need to have
> reviews posted already.
>
> At this point, if anyone has any specific new features they want
> reviewed prior to the release, please reply on this thread.
>
> Hard freeze (and creation of the branch) will be on 1 February.
>
> Thanks,
> Mark Michelson
>

Hi Mark,

I'd like to have the following series considered for the release:t
- MLD: https://patchwork.ozlabs.org/project/openvswitch/list/?series=151991
- LB Hairpinning: Not a feature, but a significant change already
under review by Numan:
https://patchwork.ozlabs.org/patch/1224295/
- stage-hint extension: also not a feature but a significant change
that improves debuggability:
https://patchwork.ozlabs.org/patch/1224889/

Thanks,
Dumitru

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

[ovs-dev] OVN Soft Freeze

[Mark Michelson]

Hi all,

OVN has entered its "soft freeze" state. That means that any new 
features that should be added prior to the next release need to have 
reviews posted already.


At this point, if anyone has any specific new features they want 
reviewed prior to the release, please reply on this thread.


Hard freeze (and creation of the branch) will be on 1 February.

Thanks,
Mark Michelson

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

[ovs-dev] Mein liebster Schatz

[Caroline Richard]

Mein liebster Schatz

  Wie geht es dir heute? Ich hoffe, dir geht es gut.

  Ich bin Fräulein Caroline Richard. Bitte ich möchte, dass Sie mir bei der
Investition eines Gesamtbetrags von (5,7 Millionen Euro) helfen, den ich
von meinen verstorbenen Eltern geerbt habe. Ich bitte um Ihre
Unterstützung. Ich habe keine Geschäftsidee. Ich möchte, dass Sie mir
helfen, diesen Fonds in Ihren zu investieren Land, während ich kommen
werde, um bei Ihnen zu bleiben und meine Ausbildung in Ihrem Land
fortzusetzen.

Bitte ich gebe Ihnen 30% des Gesamtbetrags, sobald der Betrag auf Ihr Konto
überwiesen wurde, und ich gebe Ihnen weitere Einzelheiten, sobald ich von
Ihnen höre. Ich bin bereit, Ihr Freund zu sein

Ich habe mein sexy romantisches Bild für dich angehängt

Ich hoffe, von Ihnen positiv zu hören

Mit freundlichen Grüßen

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

Re: [ovs-dev] [PATCH ovn 2/2] ovn-controller: Cache logical flow expr tree for each lflow.

[Han Zhou]

On Thu, Jan 9, 2020 at 9:37 AM  wrote:
>
> From: Numan Siddique 
>
> This patch caches the logical flow expr tree for each logical flow. This
> cache is stored as an hashmap in the output_flow engine data. When a
logical
> flow is deleted, the expr tree for that lflow is deleted in the
> flow_output_sb_logical_flow_handler().
>
> Below are the details of the OVN resource in my setup
>
> No of logical switches - 49
> No of logical ports- 1191
> No of logical routers  - 7
> No of logical ports- 51
> No of ACLs - 1221
> No of Logical flows- 664736
>
> On a chassis hosting 7 distributed router ports and around 1090
> port bindings, the no of OVS rules was 921162.
>
> Without this patch, the function add_logical_flows() took ~15 seconds.
> And with this patch it took ~10 seconds. There is a good 34% improvement
> in logical flow processing.

Great improvement! Thanks Numan. Please see my comments below.

>
> Signed-off-by: Numan Siddique 
> ---
>  controller/lflow.c  | 182 ++--
>  controller/lflow.h  |   9 +-
>  controller/ovn-controller.c |  17 +++-
>  3 files changed, 154 insertions(+), 54 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 93ec53a1c..be46f0661 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -79,7 +79,9 @@ static bool consider_logical_flow(
>  struct ovn_extend_table *group_table,
>  struct ovn_extend_table *meter_table,
>  struct lflow_resource_ref *lfrr,
> -uint32_t *conj_id_ofs);
> +uint32_t *conj_id_ofs,
> +struct hmap *lflow_expr_cache,
> +bool delete_expr_from_cache);

It was a convention that output args are at the end of the list (Ben took a
lot of effort to make it this way when we were implementing incremental
processing), so it would be better to move the last one
"delete_expr_from_cache" to the position before ovn_desired_flow_table.

>
>  static bool
>  lookup_port_cb(const void *aux_, const char *port_name, unsigned int
*portp)
> @@ -255,6 +257,59 @@ lflow_resource_destroy_lflow(struct
lflow_resource_ref *lfrr,
>  free(lfrn);
>  }
>
> +struct lflow_expr {
> +struct hmap_node node;
> +struct uuid lflow_uuid; /* key */
> +struct expr *expr;
> +char *lflow_match;
> +};
> +
> +static void
> +lflow_expr_add(struct hmap *lflow_expr_cache,
> +   const struct sbrec_logical_flow *lflow,
> +   struct expr *lflow_expr)
> +{
> +struct lflow_expr *le = xmalloc(sizeof *le);
> +le->lflow_uuid = lflow->header_.uuid;
> +le->expr = lflow_expr;
> +le->lflow_match = xstrdup(lflow->match);
> +hmap_insert(lflow_expr_cache, >node, uuid_hash(>lflow_uuid));
> +}
> +
> +static struct lflow_expr *
> +lflow_expr_get(struct hmap *lflow_expr_cache,
> +   const struct sbrec_logical_flow *lflow)
> +{
> +struct lflow_expr *le;
> +size_t hash = uuid_hash(>header_.uuid);
> +HMAP_FOR_EACH_WITH_HASH (le, node, hash, lflow_expr_cache) {
> +if (!strcmp(lflow->match, le->lflow_match)) {

I think here we should compare lflow uuid first to make sure we found the
original lflow, and then compare lflow_match to make sure the match didn't
change. If the lflow uuid matched but match string changed, it means the
cache is no longer valid and we should delete it from the cache.
Otherwise, if a lflow in SB is updated on the match field, it will end up
with unused entries in the cache forever, unless the same lflow's match
condition changed back.
In fact, the case that a lflow's match condition changes may never happen
at all considering northd's logic (correct me if I am wrong), and if this
is true, then I don't think we need to compare the lflow->match at all, and
then we don't need the lflow_match in the struct lflow_expr.
In either case, it seems this code need some change.

> +return le;
> +}
> +}
> +
> +return NULL;
> +}
> +
> +static void
> +lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
> +{
> +hmap_remove(lflow_expr_cache, >node);
> +free(le->lflow_match);
> +expr_destroy(le->expr);
> +free(le);
> +}
> +
> +void
> +lflow_expr_destroy(struct hmap *lflow_expr_cache)
> +{
> +struct lflow_expr *le;
> +HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
> +free(le->lflow_match);

I didn't see le->expr being destroyed. It seems memory leak here?

> +free(le);
> +}

It may be better to destroy the hmap in this function instead of calling
hmap_destroy separately by the caller.

> +}
> +
>  /* Adds the logical flows from the Logical_Flow table to flow tables. */
>  static void
>  add_logical_flows(
> @@ -273,7 +328,8 @@ add_logical_flows(
>  struct ovn_extend_table *group_table,
>  struct ovn_extend_table *meter_table,
>  struct lflow_resource_ref *lfrr,
> -uint32_t *conj_id_ofs)
> +uint32_t *conj_id_ofs,
> +struct hmap *lflow_expr_cache)
>  {
>  const struct 

Re: [ovs-dev] [PATCH ovn 1/2] expr: Evaluate the condition expression in a separate step.

[Han Zhou]

On Thu, Jan 9, 2020 at 9:37 AM  wrote:
>
> From: Numan Siddique 
>
> A new function is added - expr_evaluate_condition() which evaluates
> the condition expression - is_chassis_resident. expr_simply() will
> no longer evaluates this condition.
>
> An upcoming commit needs this in order to cache the logical flow
expressions
> so that every lflow_*() function which calls consider_logical_flow()
doesn't
> need to convert the logical flow match to an expression. Instead the expr
tree
> for the logical flow is cached. Since we can't cache the
is_chassis_resident
> condition, it is separated out.
>
> Signed-off-by: Numan Siddique 
> ---
>  controller/lflow.c|  3 ++-
>  include/ovn/expr.h| 10 
>  lib/expr.c| 55 +--
>  tests/test-ovn.c  | 10 
>  utilities/ovn-trace.c |  5 +++-
>  5 files changed, 60 insertions(+), 23 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index a6893201e..93ec53a1c 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -661,8 +661,9 @@ consider_logical_flow(
>  .chassis = chassis,
>  .active_tunnels = active_tunnels,
>  };
> -expr = expr_simplify(expr, is_chassis_resident_cb, _aux);
> +expr = expr_simplify(expr);
>  expr = expr_normalize(expr);
> +expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
_aux);
>  uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, ,
> );
>  expr_destroy(expr);
> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> index 21bf51c22..00a021236 100644
> --- a/include/ovn/expr.h
> +++ b/include/ovn/expr.h
> @@ -404,10 +404,12 @@ void expr_destroy(struct expr *);
>
>  struct expr *expr_annotate(struct expr *, const struct shash *symtab,
> char **errorp);
> -struct expr *expr_simplify(struct expr *,
> -   bool (*is_chassis_resident)(const void *c_aux,
> -   const char
*port_name),
> -   const void *c_aux);
> +struct expr *expr_simplify(struct expr *);
> +struct expr *expr_evaluate_condition(
> +struct expr *,
> +bool (*is_chassis_resident)(const void *c_aux,
> +const char *port_name),
> +const void *c_aux);
>  struct expr *expr_normalize(struct expr *);
>
>  bool expr_honors_invariants(const struct expr *);
> diff --git a/lib/expr.c b/lib/expr.c
> index e5e4d21b7..12557e3ca 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -2033,10 +2033,10 @@ expr_simplify_relational(struct expr *expr)
>
>  /* Resolves condition and replaces the expression with a boolean. */
>  static struct expr *
> -expr_simplify_condition(struct expr *expr,
> -bool (*is_chassis_resident)(const void *c_aux,
> +expr_evaluate_condition__(struct expr *expr,
> +  bool (*is_chassis_resident)(const void *c_aux,
>  const char
*port_name),
> -const void *c_aux)
> +  const void *c_aux)
>  {
>  bool result;
>
> @@ -2054,13 +2054,41 @@ expr_simplify_condition(struct expr *expr,
>  return expr_create_boolean(result);
>  }
>
> +struct expr *
> +expr_evaluate_condition(struct expr *expr,
> +bool (*is_chassis_resident)(const void *c_aux,
> +const char
*port_name),
> +const void *c_aux)
> +{
> +struct expr *sub, *next;
> +
> +switch (expr->type) {
> +case EXPR_T_AND:
> +case EXPR_T_OR:
> + LIST_FOR_EACH_SAFE (sub, next, node, >andor) {
> +ovs_list_remove(>node);
> +struct expr *e = expr_evaluate_condition(sub,
is_chassis_resident,
> + c_aux);
> +e = expr_fix(e);
> +expr_insert_andor(expr, next, e);
> +}
> +return expr_fix(expr);
> +
> +case EXPR_T_CONDITION:
> +return expr_evaluate_condition__(expr, is_chassis_resident,
c_aux);
> +
> +case EXPR_T_CMP:
> +case EXPR_T_BOOLEAN:
> +return expr;
> +}
> +
> +OVS_NOT_REACHED();
> +}
> +
>  /* Takes ownership of 'expr' and returns an equivalent expression whose
>   * EXPR_T_CMP nodes use only tests for equality (EXPR_R_EQ). */
>  struct expr *
> -expr_simplify(struct expr *expr,
> -  bool (*is_chassis_resident)(const void *c_aux,
> -  const char *port_name),
> -  const void *c_aux)
> +expr_simplify(struct expr *expr)
>  {
>  struct expr *sub, *next;
>
> @@ -2075,8 +2103,7 @@ expr_simplify(struct expr *expr,
>  case EXPR_T_OR:
>  LIST_FOR_EACH_SAFE (sub, next, node, >andor) {
>  ovs_list_remove(>node);
> -expr_insert_andor(expr, next,
> -