[ovs-dev] [PATCH] MAINTAINERS: Move Joe to emeritus status.

[Joe Stringer]

My primary focus has been in adjacent communities for some time now.
It seems appropriate that my status in the OVS maintainers file should
reflect the level of attention I am able to provide to the project.
Thanks to the other contributors past and present for the experiences
we've shared, and I'll see you around wherever our paths cross again :-)

Signed-off-by: Joe Stringer 
---
 MAINTAINERS.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS.rst b/MAINTAINERS.rst
index f4f6188c85cb..414df68758c1 100644
--- a/MAINTAINERS.rst
+++ b/MAINTAINERS.rst
@@ -61,8 +61,6 @@ This is the current list of active Open vSwitch committers:
  - ja...@ovn.org
* - Jesse Gross
  - je...@kernel.org
-   * - Joe Stringer
- - j...@ovn.org
* - Justin Pettit
  - jpet...@ovn.org
* - Pravin B Shelar
@@ -91,3 +89,5 @@ More information about Emeritus Committers can be found
  - b...@ovn.org
* - Ethan J. Jackson
  - e...@eecs.berkeley.edu
+   * - Joe Stringer
+ - j...@ovn.org
-- 
2.25.1

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

Re: [ovs-dev] [PATCH] openvswitch: perform refragmentation for packets which pass through conntrack

[Joe Stringer]

Hey Aaron, long time no chat :)

On Fri, Mar 19, 2021 at 1:43 PM Aaron Conole  wrote:
>
> When a user instructs a flow pipeline to perform connection tracking,
> there is an implicit L3 operation that occurs - namely the IP fragments
> are reassembled and then processed as a single unit.  After this, new
> fragments are generated and then transmitted, with the hint that they
> should be fragmented along the max rx unit boundary.  In general, this
> behavior works well to forward packets along when the MTUs are congruent
> across the datapath.
>
> However, if using a protocol such as UDP on a network with mismatching
> MTUs, it is possible that the refragmentation will still produce an
> invalid fragment, and that fragmented packet will not be delivered.
> Such a case shouldn't happen because the user explicitly requested a
> layer 3+4 function (conntrack), and that function generates new fragments,
> so we should perform the needed actions in that case (namely, refragment
> IPv4 along a correct boundary, or send a packet too big in the IPv6 case).
>
> Additionally, introduce a test suite for openvswitch with a test case
> that ensures this MTU behavior, with the expectation that new tests are
> added when needed.
>
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Signed-off-by: Aaron Conole 
> ---
> NOTE: checkpatch reports a whitespace error with the openvswitch.sh
>   script - this is due to using tab as the IFS value.  This part
>   of the script was copied from
>   tools/testing/selftests/net/pmtu.sh so I think should be
>   permissible.
>
>  net/openvswitch/actions.c  |   2 +-
>  tools/testing/selftests/net/.gitignore |   1 +
>  tools/testing/selftests/net/Makefile   |   1 +
>  tools/testing/selftests/net/openvswitch.sh | 394 +
>  4 files changed, 397 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/net/openvswitch.sh
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 92a0b67b2728..d858ea580e43 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -890,7 +890,7 @@ static void do_output(struct datapath *dp, struct sk_buff 
> *skb, int out_port,
> if (likely(!mru ||
>(skb->len <= mru + vport->dev->hard_header_len))) {
> ovs_vport_send(vport, skb, ovs_key_mac_proto(key));
> -   } else if (mru <= vport->dev->mtu) {
> +   } else if (mru) {
> struct net *net = read_pnet(>net);
>
> ovs_fragment(net, vport, skb, mru, key);

I thought about this for a while. For a bit of context, my
recollection is that in the initial design, there was an attempt to
minimize the set of assumptions around L3 behaviour and despite
performing this pseudo-L3 action of connection tracking, attempt a
"bump-in-the-wire" approach where OVS is serving as an L2 switch and
if you wanted L3 features, you need to build them on top or explicitly
define that you're looking for L3 semantics. In this case, you're
interpreting that the combination of the conntrack action + an output
action implies that L3 routing is being performed. Hence, OVS should
act like a router and either refragment or generate ICMP PTB in the
case where MTU differs. According to the flow table, the rest of the
routing functionality (MAC handling for instance) may or may not have
been performed at this point, but we basically leave that up to the
SDN controller to implement the right behaviour. In relation to this
particular check, the idea was to retain the original geometry of the
packet such that it's as though there were no functionality performed
in the middle at all. OVS happened to do connection tracking (which
implicitly involved queueing fragments), but if you treat it as an
opaque box, you have ports connected and OVS is simply performing
forwarding between the ports.

One of the related implications is the contrast between what happens
in this case if you have a conntrack action injected or not when
outputting to another port. If you didn't put a connection tracking
action into the flows when redirecting here, then there would be no
defragmentation or refragmentation. In that case, OVS is just
attempting to forward to another device and if the MTU check fails,
then bad luck, packets will be dropped. Now, with the interpretation
in this patch, it seems like we're trying to say that, well, actually,
if the controller injects a connection tracking action, then the
controller implicitly switches OVS into a sort of half-L3 mode for
this particular flow. This makes the behaviour a bit inconsistent.

Another thought that occurs here is that if you have three interfaces
attached to the switch, say one with MTU 1500 and two with MTU 1450,
and the OVS flows are configured to conntrack and clone the packets
from the higher-MTU interface to the lower-MTU interfaces. If you
receive 

Re: [ovs-dev] openvswitch crash on i386

[Joe Stringer]

On Tue, Mar 5, 2019 at 2:12 AM Christian Ehrhardt
 wrote:
>
> On Tue, Mar 5, 2019 at 10:58 AM Juerg Haefliger
>  wrote:
> >
> > Hi,
> >
> > Running the following commands in a loop will crash an i386 5.0 kernel
> > typically within a few iterations:
> >
> > ovs-vsctl add-br test
> > ovs-vsctl del-br test
> >
> > [  106.215748] BUG: unable to handle kernel paging request at e8a35f3b
> > [  106.216733] #PF error: [normal kernel read fault]
> > [  106.217464] *pdpt = 19a76001 *pde = 
> > [  106.218346] Oops:  [#1] SMP PTI
> > [  106.218911] CPU: 0 PID: 2050 Comm: systemd-udevd Tainted: GE 
> > 5.0.0 #25
> > [  106.220103] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> > 1.11.1-1ubuntu1 04/01/2014
> > [  106.221447] EIP: kmem_cache_alloc_trace+0x7a/0x1b0
> > [  106.222178] Code: 01 00 00 8b 07 64 8b 50 04 64 03 05 28 61 e8 d2 8b 08 
> > 89 4d ec 85 c9 0f 84 03 01 00 00 8b 45 ec 8b 5f 14 8d 4a 01 8b 37 01 c3 
> > <33> 1b 33 9f b4 00 00 00 64 0f c7 0e 75 cb 8b 75 ec 8b 47 14 0f 18
> > [  106.224752] EAX: e8a35f3b EBX: e8a35f3b ECX: 869f EDX: 869e
> > [  106.225683] ESI: d2e96ef0 EDI: da401a00 EBP: d9b85dd0 ESP: d9b85db0
> > [  106.226662] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010282
> > [  106.227710] CR0: 80050033 CR2: e8a35f3b CR3: 185b8000 CR4: 06f0
> > [  106.228703] DR0:  DR1:  DR2:  DR3: 
> > [  106.229604] DR6: fffe0ff0 DR7: 0400
> > [  106.230114] Call Trace:
> > [  106.230525]  ? kernfs_fop_open+0xb4/0x390
> > [  106.231176]  kernfs_fop_open+0xb4/0x390
> > [  106.231856]  ? security_file_open+0x7c/0xc0
> > [  106.232562]  do_dentry_open+0x131/0x370
> > [  106.233229]  ? kernfs_fop_write+0x180/0x180
> > [  106.233905]  vfs_open+0x25/0x30
> > [  106.234432]  path_openat+0x2fd/0x1450
> > [  106.235084]  ? cp_new_stat64+0x115/0x140
> > [  106.235754]  ? cp_new_stat64+0x115/0x140
> > [  106.236427]  do_filp_open+0x6a/0xd0
> > [  106.237026]  ? cp_new_stat64+0x115/0x140
> > [  106.237748]  ? strncpy_from_user+0x3d/0x180
> > [  106.238539]  ? __alloc_fd+0x36/0x120
> > [  106.239256]  do_sys_open+0x175/0x210
> > [  106.239955]  sys_openat+0x1b/0x20
> > [  106.240596]  do_fast_syscall_32+0x7f/0x1e0
> > [  106.241313]  entry_SYSENTER_32+0x6b/0xbe
> > [  106.242017] EIP: 0xb7fae871
> > [  106.242559] Code: 8b 98 58 cd ff ff 89 c8 85 d2 74 02 89 0a 5b 5d c3 8b 
> > 04 24 c3 8b 14 24 c3 8b 34 24 c3 8b 3c 24 c3 51 52 55 89 e5 0f 34 cd 80 
> > <5d> 5a 59 c3 90 90 90 90 8d 76 00 58 b8 77 00 00 00 cd 80 90 8d 76
> > [  106.245551] EAX: ffda EBX: ff9c ECX: bffdcb60 EDX: 00088000
> > [  106.246651] ESI:  EDI: b7f9e000 EBP: 00088000 ESP: bffdc970
> > [  106.247706] DS: 007b ES: 007b FS:  GS: 0033 SS: 007b EFLAGS: 0246
> > [  106.248851] Modules linked in: openvswitch(E)
> > [  106.249621] CR2: e8a35f3b
> > [  106.250218] ---[ end trace 6a8d05679a59cda7 ]---
> >
> > I've bisected this down to the following commit that seems to have 
> > introduced
> > the issue:
> >
> > commit 120645513f55a4ac5543120d9e79925d30a0156f (refs/bisect/bad)
> > Author: Jarno Rajahalme 
> > Date:   Fri Apr 21 16:48:06 2017 -0700
> >
> > openvswitch: Add eventmask support to CT action.
> >
> > Add a new optional conntrack action attribute OVS_CT_ATTR_EVENTMASK,
> > which can be used in conjunction with the commit flag
> > (OVS_CT_ATTR_COMMIT) to set the mask of bits specifying which
> > conntrack events (IPCT_*) should be delivered via the Netfilter
> > netlink multicast groups.  Default behavior depends on the system
> > configuration, but typically a lot of events are delivered.  This can be
> > very chatty for the NFNLGRP_CONNTRACK_UPDATE group, even if only some
> > types of events are of interest.
> >
> > Netfilter core init_conntrack() adds the event cache extension, so we
> > only need to set the ctmask value.  However, if the system is
> > configured without support for events, the setting will be skipped due
> > to extension not being found.
> >
> > Signed-off-by: Jarno Rajahalme 
> > Reviewed-by: Greg Rose 
> > Acked-by: Joe Stringer 
> > Signed-off-by: David S. Miller 
>
> Hi Juerg,
> the symptom, the identified breaking commit and actually all of it
> seems to be [1] which James, Joseph and I worked on already.
> I wanted to make you aware of the past context that already exists.
>

Re: [ovs-dev] [PATCH] Revert "openvswitch: Fix template leak in error cases."

[Joe Stringer]

On Fri, 28 Sep 2018 at 10:55, Flavio Leitner  wrote:
>
> This reverts commit 90c7afc96cbbd77f44094b5b651261968e97de67.
>
> When the commit was merged, the code used nf_ct_put() to free
> the entry, but later on commit 76644232e612 ("openvswitch: Free
> tmpl with tmpl_free.") replaced that with nf_ct_tmpl_free which
> is a more appropriate. Now the original problem is removed.
>
> Then 44d6e2f27328 ("net: Replace NF_CT_ASSERT() with WARN_ON().")
> replaced a debug assert with a WARN_ON() which is trigged now.
>
> Signed-off-by: Flavio Leitner 
> ---

Thanks for the cleanup.

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

Re: [ovs-dev] [PATCH] ofproto-dpif: Init ukey->dump_seq to zero

[Joe Stringer]

On 6 April 2018 at 10:44, Ben Pfaff <b...@ovn.org> wrote:
> [also CCing Joe on the chance that he wants to comment]
>
> On Fri, Apr 06, 2018 at 09:34:38AM +, Jan Scheurich wrote:
>> > -Original Message-
>> > From: Ben Pfaff [mailto:b...@ovn.org]
>> > Sent: Wednesday, 04 April, 2018 22:28
>> >
>> > Oh, that's weird.  It's as if I didn't read the patch.  Maybe I just
>> > read some preliminary version in another thread.
>> >
>> > Anyway, you're totally right.  I applied this to master.  If you're
>> > seeing problems in another branch, let me know and I will backport.
>>
>> Thanks!
>>
>> I think the issue was introduced into OVS by the following commit a long 
>> time ago.
>>
>> commit 23597df052262dec961fd86eb7c54d10984a1ec0
>> Author: Joe Stringer <joestrin...@nicira.com>
>> Date:   Fri Jul 25 13:54:24 2014 +1200
>>
>> It's a temporary glitch that can cause unexpected behavior only within
>> the first few hundred milliseconds after datapath flow creation. It is
>> most likely to affect "reactive" controller use cases (MAC learning,
>> ARP handling), like the OVN test case that now failed with a small
>> change of timing. So it is possible that one could notice short packet
>> drops or duplicate PACKET_INs in real SDN deployments when looking
>> close enough.

I see, so there's two seqs used in this code, dump_seq and reval_seq.
The purpose of the dump_seq is just to handle the case where a thread
dumps the exact same flow from the kernel datapath twice in quick
succession. This may happen because the kernel API doesn't provide a
guarantee about dumping an exact snapshot of the entire table, so if
one set of flows (~10-15) are dumped from the kernel, then a flow is
inserted into the bucket that is being dumped right now, then a
subsequent dump will begin from the same bucket/index as where the
previous dump ended, and perhaps rather than starting from the very
next flow, it starts with the last flow of the previous dump. Hence we
see the same flow twice.

>From this perspective, setting the dump_seq during upcall processing
does not make sense, so I agree with the basis of the patch. However,
I note that you also set it to 0 in ukey_create_from_dpif_flow(),
which is actually a dump so the same flow could be revalidated twice.
This doesn't quite seem right according to the above reasoning, but
that said looking over the code I can't see anything problematic with
handling it this way. Creating ukeys from dpif_flows is very much a
corner case and it seems unlikely to have a large impact even if there
is some logical mismatch in this case. (Equally, because it's such a
corner case, people are far less likely to notice a problem here..)

If I follow the order of relevant executions is something like:

* Openflow has set of rules R1
* dump_seq is set to N1
* new datapath flow arrives, creates upcall with N1 and associated
ukey with same dump_seq
* datapath flow is forwarded to controller, controller pushes new
openflow flow, rules are now R2
* during the same dump, the revalidator dumps the newly installed
flow, finds the ukey, sees the dump_seq N1 and skips revalidating it
although there are new rules R2
* traffic continues to be forwarded according to R1 until the next dump
* Because of rule transition R1->R2, another flow dump / revalidation
round is immediately triggered via reval_seq, with minimum bound of
starting in T+5ms
* dump_seq is set to N2
* During this second revalidation round, the datapath flow is actually
checked, determined to be forwarding per out-of-date behaviour, and
updated / evicted.

I would think that if you have a small number of flows, a new
revalidation round should be triggered fairly quickly due to reval_seq
(scheduled to happen in minimum ~5ms, as per updif_revalidator()). In
this window the flow will not be revalidated so you would still see
blips like this. On the other hand, if you have a large number of
flows, statistically you may not end up dumping the newly installed
flow during the same dump, so it may still take hundreds of
milliseconds for the dump to finish, to trigger revalidation due to
the R1->R2 rule change, then for the flow to eventually be revalidated
to implement the new behaviour. I don't know that it would make a big
difference there. That said, for the case where the flow /is/ dumped
during the same round, its actions may be updated so OVS would be more
responsive to OpenFlow changes in that case.

If you think there's a slightly different timeline, I'd be curious of
your thoughts on it though I won't provide any guarantee I'll be able
to provide further insight into it :-)

I believe that you observe an improvement in behaviour with the patch,
so I have no objections. Logically it seems the right direction. That
said

Re: [ovs-dev] [PATCH 1/4] ovs-kmod-ctl: introduce a kernel module load script

[Joe Stringer]

On 26 March 2018 at 14:32, Aaron Conole  wrote:
> Thanks for the review, Ansis!
>
> Ansis Atteka  writes:
>
>> On 20 March 2018 at 14:05, Aaron Conole  wrote:
>>> +.
>>> +.PP
>>> +Each of \fBovs\-ctl\fR's commands is described separately below.
>>> +.
>>> +.SH "The ``insert'' command"
>>> +.
>>> +.PP
>>> +The \fBinsert\fR command loads the Open vSwitch kernel modules, if
>>> +needed.  If this fails, and the Linux bridge module is loaded but no
>>> +bridges exist, it tries to unload the bridge module and tries loading
>>> +the Open vSwitch kernel module again. (This is because the Open
>>> +vSwitch kernel module cannot coexist with the Linux bridge module
>>> +before 2.6.37.)
>> FYI: I believe latest Open vSwitch does not support kernels older than 3.10:
>
> Sure.  I can take the parenthetical phrase out.
>
>>AC_ERROR([Linux kernel in $KBUILD is version $kversion, but
>> version 3.10 or later is required])

FWIW this isn't supported kernel versions, this is only versions that
the module in the tree will compile against. In theory people can
still use older kernels with the latest OVS userspace (though some
features may be missing).
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] try_ukey_replace() try lock strategy

[Joe Stringer]

On 20 March 2018 at 06:12, Eelco Chaudron  wrote:
> Hi Joe,
>
> I'm investigating an issue where I've seen "handler_duplicate_upcall"
> incrementing once, and where "upcall_ukey_replace" is happening quite often.

Without other evidence of undesirable behaviour, I wouldn't be overly
concerned about "handler_duplicate_upcall" happening once. All it
takes is for two packets of the same flow to be sent to userspace at
once for handling, perhaps because userspace is a little slow to
handle the first upcall, or because the sender transmits a couple of
packets in quick succession so they arrive and are handled by the
datapath at close timing, and therefore are queued to userspace at the
same time.

"upcall_ukey_replace" recurrence could be a bit more interesting, but
I'd have to think further about it. Simplest possibility that comes to
mind is that revalidators are deleting flows a bit too aggressively,
so a flow that's actively (or just about to be) receiving traffic is
flushed from the datapath, then handlers receive upcalls for the flows
soon after. If you've got a lot of flows in the datapath, I could see
this happening. A less likely option would be some weird interaction
with a datapath that violates the guarantee that the datapath dumps
flows exactly the same way that they were inserted.

"ovs-appctl upcall/show" might help a bit with digging into this.

> This all relates to the try_ukey_replace() function:
>
> +static bool
> +try_ukey_replace(struct umap *umap, struct udpif_key *old_ukey,
> + struct udpif_key *new_ukey)
> +OVS_REQUIRES(umap->mutex)
> +OVS_TRY_LOCK(true, new_ukey->mutex)
> +{
> +bool replaced = false;
> +
> +if (!ovs_mutex_trylock(_ukey->mutex)) {
> +if (old_ukey->state == UKEY_EVICTED) {
> +/* The flow was deleted during the current revalidator dump,
> + * but its ukey won't be fully cleaned up until the sweep
> phase.
> + * In the mean time, we are receiving upcalls for this traffic.
> + * Expedite the (new) flow install by replacing the ukey. */
> +ovs_mutex_lock(_ukey->mutex);
> +cmap_replace(>cmap, _ukey->cmap_node,
> + _ukey->cmap_node, new_ukey->hash);
> +ovsrcu_postpone(ukey_delete__, old_ukey);
> +transition_ukey(old_ukey, UKEY_DELETED);
> +transition_ukey(new_ukey, UKEY_VISIBLE);
> +replaced = true;
> +}
> +ovs_mutex_unlock(_ukey->mutex);
> +}
> +
> +if (replaced) {
> +COVERAGE_INC(upcall_ukey_replace);
> +} else {
> +COVERAGE_INC(handler_duplicate_upcall);
> +}
> +return replaced;
> +}
> +
>
>
>
> Currently, there is no debugging in place to get an idea if the lock failed
> or the wrong state it was in. I'll instrument the code if we can get it to
> fail again/reliably.
>
> In the meantime, I was wondering why we do an ovs_mutex_trylock(), and not
> just a lock()?

The trylock() is there because other threads may handle the exact same
flow at the exact same time, and only one of them needs to actually
handle (eg, delete / attribute stats for) it. IIRC, this typically
does not happen concurrently in handler threads because an upcall for
a particular flow is likely hashed to the same netlink socket (so only
one handler thread would attempt to handle an upcall for this flow at
the same time, though it could receive two upcalls for the same flow
in succession). However for revalidator threads it is quite possible
for the same flow to be dumped twice when a flow is being deleted from
the kernel, as the kernel flow dump API doesn't provide a guarantee
that the entire table will be dumped exactly as it is, with one
instance of each flow.

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

Re: [ovs-dev] [PATCH v1 1/2] userspace datapath: Add GTP-U tunnel support

[Joe Stringer]

Ok, thanks for the clarification.

On Mon, 5 Mar 2018, 03:23 Yang, Yi, <yi.y.y...@intel.com> wrote:

> On Mon, Mar 05, 2018 at 04:19:58AM +0800, Joe Stringer wrote:
> > ?On 2 March 2018 at 04:32, Yi Yang <yi.y.y...@intel.com> wrote:
> > > From: Feng Yang <feng.y...@intel.com>
> > >
> > > +
> > > +csum = csum_continue(csum, udp, ip_tot_size);
> > > +udp->udp_csum = csum_finish(csum);
> > > +
> > > +if (!udp->udp_csum) {
> > > +udp->udp_csum = htons(0x);
> >
> > What's the motivation behind this?
>
> More comments, this is from the existing ovs code, Feng checked why.
>
> [Feng] According to the standard RFC768,
> https://tools.ietf.org/html/rfc768, I quote, "if the computed  checksum
> is zero,  it is transmitted  as all ones (the equivalent  in one's
> complement  arithmetic)". Actually, netdev_tnl_udp_push_header( ) did
> the same.
>
> >
> > > +}
> > > +}
> > > +
> > > +packet->packet_type = htonl(PT_ETH);
> > > +}
> > > +
> > > +
> > > +int
> > > +netdev_gtpu_build_header(const struct netdev *netdev,
> > > +  struct ovs_action_push_tnl *data,
> > > +  const struct netdev_tnl_build_header_params
> *params)
> > > +{
> > > +struct netdev_vport *dev = netdev_vport_cast(netdev);
> > > +struct netdev_tunnel_config *tnl_cfg;
> > > +struct gtpuhdr *gtph;
> > > +struct udp_header *udp;
> > > +
> > > +/* XXX: RCUfy tnl_cfg. */
> > > +ovs_mutex_lock(>mutex);
> > > +
> > > +tnl_cfg = >tnl_cfg;
> > > +udp = netdev_tnl_ip_build_header(data, params, IPPROTO_UDP);
> > > +udp->udp_dst = tnl_cfg->dst_port;
> > > +udp->udp_src = htons(GTPU_DST_PORT);
> > > +
> > > +if (params->is_ipv6 || params->flow->tunnel.flags &
> FLOW_TNL_F_CSUM) {
> > > +/* Write a value in now to mark that we should compute the
> checksum
> > > + * later. 0x is handy because it is transparent to the
> > > + * calculation. */
> > > +udp->udp_csum = htons(0x);
> >
> > Maybe I missed it, but I didn't see where in this patch that it is
> > computed later due to this value.
> >
>
> This is also from the existing code. udp_build_header is doing same
> thing.
>
> [Feng]  udp_csum is calculated in netdev_gtpu_push_header( ). In
> principle, udp_csum can be arbitrarily set to any two octets except all
> zero, which means transmitter generated no checksum, according to the
> standard, https://tools.ietf.org/html/rfc768.
> We followed what udp_build_header( ) did, by setting udp_csum to 0x.
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1 1/2] userspace datapath: Add GTP-U tunnel support

[Joe Stringer]

On 4 March 2018 at 19:58, Yang, Yi <yi.y.y...@intel.com> wrote:
> On Sun, Mar 04, 2018 at 07:40:49PM -0800, Joe Stringer wrote:
>> On 4 March 2018 at 18:48, Yang, Yi <yi.y.y...@intel.com> wrote:
>> > On Mon, Mar 05, 2018 at 04:19:58AM +0800, Joe Stringer wrote:
>> >> ?On 2 March 2018 at 04:32, Yi Yang <yi.y.y...@intel.com> wrote:
>> >> > +goto err;
>> >> > +}
>> >> > +} else {
>> >> > +/* tnl->gtpu_msgtype can identify if it is GTP-U message,
>> >> > + * miniflow_extract won't parse it if tnl->gtpu_msgtype
>> >> > + * isn't equal to GTPU_MSGTYPE_GPDU.
>> >> > + */
>> >> > +packet->packet_type = htonl(PT_UNKNOWN);
>> >>
>> >> This is where the limits of my GTP understanding will show, I was
>> >> under the impression that GTP-U provides encapsulation for user
>> >> traffic, so would commonly encapsulate IPv4/IPv6 inside. Whereas for
>> >> GTP-C, it's signalling traffic so would not have such inner headers -
>> >> it's supposed to be handled by a GTP implementation. Why is it
>> >> important to be able to decapsulate GTP-C? Furthermore, what's
>> >> ensuring that we don't end up spitting inner GTP-C packet data onto
>> >> the wire without headers post-translation?
>> >>
>> > GTP-C is for control path, so I don't think it makes sense for OVS to
>> > handle this, in addition GTP-C used different UDP port from GTP-U's, so
>> > we must have different tunnel ports to handle them respectively.
>>
>> Re: GTP-C in OVS, I'm inclined to agree that it doesn't make sense to
>> be in OVS. There's a lot of complexity there and existing FOSS
>> projects already attempt to support this, such as those under Osmocom.
>>
>> The thing I was trying to highlight with the above comment is that
>> this function appears to be attempting to handle both GTP-U and GTP-C,
>> but if it doesn't make sense for OVS to handle it then I'm not sure
>> why this code attempts to do so. I don't think that expecting people
>> to just send the right kind of traffic on the designated ports (eg,
>> GTP-U on the GTP-U port) is enough, these paths need to be resilient
>> to unexpected input. Perhaps it's enough to restrict installation of
>> flows with pop GTP actions based on presence of the appropriate GTP
>> msgtype in the flow key.
>
> GTP-U has two kinds of message types, normal GTP-U PDU and signaling
> messages, so we have to handle signaling messages here, it isn't GTP-C,
> http://www.3gpp.org/ftp/Specs/archive/29_series/29.281/29281-f10.zip
> has detailed description.

I see, thanks for the reference.

> By the way, Feng said the existing GTP kernel implementation also didn't
> implement GTP-C and didn't handle other optional flag bits.

Ah, okay. The thing I was thinking of was the way to configure a GTP
socket to forward signalling traffic to a userspace process for
handling:

https://www.kernel.org/doc/Documentation/networking/gtp.txt

>>
>> > I don't know if Jiannan has implemented GTP-C support in kernel data
>> > path, we can add GTP-C support if it is really needed, but I think it is
>> > a nice-to-have thing.
>>
>> I believe there is some kind of implementation already in the kernel,
>> but you'd have to take a look to see whether its design fits with what
>> you're trying to do.
>>
>> Cheers,
>> Joe
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1 1/2] userspace datapath: Add GTP-U tunnel support

[Joe Stringer]

On 4 March 2018 at 18:48, Yang, Yi <yi.y.y...@intel.com> wrote:
> On Mon, Mar 05, 2018 at 04:19:58AM +0800, Joe Stringer wrote:
>> ?On 2 March 2018 at 04:32, Yi Yang <yi.y.y...@intel.com> wrote:
>> >
>> > Signed-off-by: Feng Yang <feng.y...@intel.com>
>> > Signed-off-by: Yi Yang <yi.y.y...@intel.com>
>> > ---
>>
>> Hi Yi, thanks for this work. Some minor comments below, but this is
>> just a "drive-by" review. I hope that others with more experience with
>> GTP will take a look as well.
>>
>> I see the comment that there is locking on the device whenever a GTP
>> header is built, I assume you intend to address that in a followup
>> iteration.
>
> Joe, thank for your comments, can you elaborate "there is locking on the
> device whenever a GTP  header is built" with more details? I don't know
> this, or there is any existing document for this? We'll fix it in next
> iteration.

See the comment " /* XXX: RCUfy tnl_cfg. */ " and related locking in the patch.

>>
>> If you haven't already, it may be interesting to sync with Jiannan
>> (CC'd) who has previously done some work on GTP support in OVS for
>> kernel, to see whether the API (OpenFlow, OVSDB) makes sense for a
>> cross-datapath (ie kernel AND userspace) implementation perspective.
>
> Yes, sync is necessary, we contacted Jiannan before, I'll ping him to
> check the status of his kernel patch, we'll make sure they can work well
> together.

Awesome.

>> > +}
>> > +tnl->gtpu_msgtype = gtph->msgtype;
>> > +tnl->tun_id = htonll(ntohl(get_16aligned_be32(>teid)));
>> > +
>> > +/*Figure out whether the inner packet is IPv4, IPv6 or a GTP-U 
>> > message.*/
>> > +if (tnl->gtpu_msgtype == GTPU_MSGTYPE_GPDU) {
>> > +struct ip_header *ip;
>> > +
>> > +ip = (struct ip_header *)(gtph + 1);
>> > +if (IP_VER(ip->ip_ihl_ver) == 4) {
>> > +packet->packet_type = htonl(PT_IPV4);
>> > +} else if (IP_VER(ip->ip_ihl_ver) == 6) {
>> > +packet->packet_type = htonl(PT_IPV6);
>> > +} else {
>> > +goto err;
>> > +}
>> > +} else {
>> > +/* tnl->gtpu_msgtype can identify if it is GTP-U message,
>> > + * miniflow_extract won't parse it if tnl->gtpu_msgtype
>> > + * isn't equal to GTPU_MSGTYPE_GPDU.
>> > + */
>> > +packet->packet_type = htonl(PT_UNKNOWN);
>>
>> This is where the limits of my GTP understanding will show, I was
>> under the impression that GTP-U provides encapsulation for user
>> traffic, so would commonly encapsulate IPv4/IPv6 inside. Whereas for
>> GTP-C, it's signalling traffic so would not have such inner headers -
>> it's supposed to be handled by a GTP implementation. Why is it
>> important to be able to decapsulate GTP-C? Furthermore, what's
>> ensuring that we don't end up spitting inner GTP-C packet data onto
>> the wire without headers post-translation?
>>
> GTP-C is for control path, so I don't think it makes sense for OVS to
> handle this, in addition GTP-C used different UDP port from GTP-U's, so
> we must have different tunnel ports to handle them respectively.

Re: GTP-C in OVS, I'm inclined to agree that it doesn't make sense to
be in OVS. There's a lot of complexity there and existing FOSS
projects already attempt to support this, such as those under Osmocom.

The thing I was trying to highlight with the above comment is that
this function appears to be attempting to handle both GTP-U and GTP-C,
but if it doesn't make sense for OVS to handle it then I'm not sure
why this code attempts to do so. I don't think that expecting people
to just send the right kind of traffic on the designated ports (eg,
GTP-U on the GTP-U port) is enough, these paths need to be resilient
to unexpected input. Perhaps it's enough to restrict installation of
flows with pop GTP actions based on presence of the appropriate GTP
msgtype in the flow key.

> I don't know if Jiannan has implemented GTP-C support in kernel data
> path, we can add GTP-C support if it is really needed, but I think it is
> a nice-to-have thing.

I believe there is some kind of implementation already in the kernel,
but you'd have to take a look to see whether its design fits with what
you're trying to do.

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

Re: [ovs-dev] [PATCH v1 1/2] userspace datapath: Add GTP-U tunnel support

[Joe Stringer]

On 4 March 2018 at 12:19, Joe Stringer <j...@ovn.org> wrote:
> ?On 2 March 2018 at 04:32, Yi Yang <yi.y.y...@intel.com> wrote:
>> From: Feng Yang <feng.y...@intel.com>
>>
>> GPRS Tunneling Protocol (GTP) is a group of IP-based communications
>> protocols used to carry general packet radio service (GPRS) within
>> GSM, UMTS and LTE networks.
>>
>> GTP can be decomposed into separate protocols, GTP-C, GTP-U and GTP'.
>>
>> 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.
>>
>> GTP-U is user plane communication protocol between Serving-Gateway
>> (S-GW) and PDN-Gateway (P-GW) as well as Multi-access Edge Computing
>> (MEC).
>>
>> This patch added a new tunnel type 'gtpu' into userspace datapath and
>> two new match fields tun_gtpu_flags and tun_gtpu_msgtype, tun_id is
>> reused for GTP Tunnel Endpoint ID (TEID).
>>
>> The below are commands for gtpu tunnel add:
>>
>> $ sudo ovs-vsctl add-br br-int -- set bridge br-int datapath_type=netdev 
>> protocols=OpenFlow13
>> $ sudo ovs-vsctl add-port br-int gtpu1 -- set interface gtpu1 type=gtpu 
>> options:packet_type=legacy_l3 options:remote_ip=flow options:key=flow 
>> options:dst_port=2152
>>
>> test-flows-gtpu.txt is openflow table for gtpu tunnel test:
>>
>> $ cat test-flows-gtpu.txt
>> table=0,icmp,in_port=4 
>> actions=load:0xc0a83249->NXM_NX_TUN_IPV4_DST[],load:0x9->NXM_NX_TUN_ID[0..31],output:2
>> table=0,tcp,tp_dst=80,in_port=4 
>> actions=load:0xc0a83249->NXM_NX_TUN_IPV4_DST[],load:0x9->NXM_NX_TUN_ID[0..31],output:2
>> table=0,in_port=2,tun_id=0x10,tun_gtpu_flags=0x30,tun_gtpu_msgtype=255 
>> actions=set_field:00:00:22:22:22:22->eth_src,set_field:00:00:11:11:11:11->eth_dst,output:4
>> table=0,actions=NORMAL
>>
>> Signed-off-by: Feng Yang <feng.y...@intel.com>
>> Signed-off-by: Yi Yang <yi.y.y...@intel.com>
>> ---
>
> Hi Yi, thanks for this work. Some minor comments below, but this is
> just a "drive-by" review. I hope that others with more experience with
> GTP will take a look as well.

Realised Feng was the author, thanks to you too!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] netlink, ovs-datapath

[Joe Stringer]

On 4 March 2018 at 00:31, Damo Brisbane  wrote:
> Hello;
>
> I am trying to do basic ovs but confused by vswitchd messages, could be
> platform issue, but having hard time trying to decipher. Am running 4.15.x
> kernel on gentoo, so going with latest github (openvswitch/ovs).
>
> Can fetch sources, run "./boot.sh && make && make install", no problem.
>
> Running ovsdb - the database path comes from other gentoo ovs installers
> (v2.8.1): /var/run/openvswitch
>
> Can anyone help enlighten me, what is the meaning of this,
> "..dpif_netlink|WARN|Generic Netlink family 'ovs_datapath' does not
> exist..."?.. ie I compiled packages, installed it, kernel recompile/reboot
> seems fine.
>
> Screendump
> --
>
> /usr/sbin/ovs-vswitchd --pidfile --monitor --mlockall
> unix:/var/run/openvswitch/db.sock
> 2018-03-04T08:14:19Z|1|ovs_numa|INFO|Discovered 8 CPU cores on NUMA
> node 0
> 2018-03-04T08:14:19Z|2|ovs_numa|INFO|Discovered 1 NUMA nodes and 8 CPU
> cores
> 2018-03-04T08:14:19Z|3|reconnect|INFO|unix:/var/run/openvswitch/db.sock:
> connecting...
> 2018-03-04T08:14:19Z|4|reconnect|INFO|unix:/var/run/openvswitch/db.sock:
> connected
> 2018-03-04T08:14:19Z|5|dpif_netlink|WARN|Generic Netlink family
> 'ovs_datapath' does not exist. The Open vSwitch kernel module is probably
> not loaded.
> 2018-03-04T08:14:19Z|6|dpif|WARN|failed to enumerate system datapaths:
> No such file or directory
> 2018-03-04T08:14:19Z|7|dpif|WARN|failed to create datapath ovs-system:
> No such file or directory
> 2018-03-04T08:14:19Z|8|ofproto_dpif|ERR|failed to open datapath of type
> system: No such file or directory
> 2018-03-04T08:14:19Z|9|ofproto|ERR|failed to open datapath br0: No such
> file or directory
> 2018-03-04T08:14:19Z|00010|bridge|ERR|failed to create bridge br0: No such
> file or directory
> 2018-03-04T08:14:19Z|00011|bridge|INFO|ovs-vswitchd (Open vSwitch) 2.9.90


Hi Damo,

The end of that line -- "The Open vSwitch kernel module is probably
not loaded" seems likely to be the culprit here. Running "make
install" from OVS will not provide the kernel module. This needs to
either be provided by the kernel as a module or built-in, or you need
to "make module_install".

For more information, see the general install guide:

http://docs.openvswitch.org/en/latest/intro/install/general/

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

Re: [ovs-dev] [PATCH v1 1/2] userspace datapath: Add GTP-U tunnel support

[Joe Stringer]

?On 2 March 2018 at 04:32, Yi Yang  wrote:
> From: Feng Yang 
>
> GPRS Tunneling Protocol (GTP) is a group of IP-based communications
> protocols used to carry general packet radio service (GPRS) within
> GSM, UMTS and LTE networks.
>
> GTP can be decomposed into separate protocols, GTP-C, GTP-U and GTP'.
>
> 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.
>
> GTP-U is user plane communication protocol between Serving-Gateway
> (S-GW) and PDN-Gateway (P-GW) as well as Multi-access Edge Computing
> (MEC).
>
> This patch added a new tunnel type 'gtpu' into userspace datapath and
> two new match fields tun_gtpu_flags and tun_gtpu_msgtype, tun_id is
> reused for GTP Tunnel Endpoint ID (TEID).
>
> The below are commands for gtpu tunnel add:
>
> $ sudo ovs-vsctl add-br br-int -- set bridge br-int datapath_type=netdev 
> protocols=OpenFlow13
> $ sudo ovs-vsctl add-port br-int gtpu1 -- set interface gtpu1 type=gtpu 
> options:packet_type=legacy_l3 options:remote_ip=flow options:key=flow 
> options:dst_port=2152
>
> test-flows-gtpu.txt is openflow table for gtpu tunnel test:
>
> $ cat test-flows-gtpu.txt
> table=0,icmp,in_port=4 
> actions=load:0xc0a83249->NXM_NX_TUN_IPV4_DST[],load:0x9->NXM_NX_TUN_ID[0..31],output:2
> table=0,tcp,tp_dst=80,in_port=4 
> actions=load:0xc0a83249->NXM_NX_TUN_IPV4_DST[],load:0x9->NXM_NX_TUN_ID[0..31],output:2
> table=0,in_port=2,tun_id=0x10,tun_gtpu_flags=0x30,tun_gtpu_msgtype=255 
> actions=set_field:00:00:22:22:22:22->eth_src,set_field:00:00:11:11:11:11->eth_dst,output:4
> table=0,actions=NORMAL
>
> Signed-off-by: Feng Yang 
> Signed-off-by: Yi Yang 
> ---

Hi Yi, thanks for this work. Some minor comments below, but this is
just a "drive-by" review. I hope that others with more experience with
GTP will take a look as well.

I see the comment that there is locking on the device whenever a GTP
header is built, I assume you intend to address that in a followup
iteration.

If you haven't already, it may be interesting to sync with Jiannan
(CC'd) who has previously done some work on GTP support in OVS for
kernel, to see whether the API (OpenFlow, OVSDB) makes sense for a
cross-datapath (ie kernel AND userspace) implementation perspective.

> diff --git a/datapath/linux/compat/include/linux/openvswitch.h 
> b/datapath/linux/compat/include/linux/openvswitch.h
> index 84ebcaf..ad6ea64 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -236,6 +236,7 @@ enum ovs_vport_type {
> OVS_VPORT_TYPE_GRE,  /* GRE tunnel. */
> OVS_VPORT_TYPE_VXLAN,/* VXLAN tunnel. */
> OVS_VPORT_TYPE_GENEVE,   /* Geneve tunnel. */
> +   OVS_VPORT_TYPE_GTPU, /* GTPU tunnel. */
> OVS_VPORT_TYPE_LISP = 105,  /* LISP tunnel */
> OVS_VPORT_TYPE_STT = 106, /* STT tunnel */
> __OVS_VPORT_TYPE_MAX
> @@ -394,6 +395,8 @@ enum ovs_tunnel_key_attr {
> OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS, /* Nested OVS_VXLAN_EXT_* */
> OVS_TUNNEL_KEY_ATTR_IPV6_SRC,   /* struct in6_addr src IPv6 
> address. */
> OVS_TUNNEL_KEY_ATTR_IPV6_DST,   /* struct in6_addr dst IPv6 
> address. */
> +   OVS_TUNNEL_KEY_ATTR_GTPU_FLAGS, /* u8 GTP-U FLAGS. */
> +   OVS_TUNNEL_KEY_ATTR_GTPU_MSGTYPE,   /* u8 GTP-U MSGTYPE. */
> OVS_TUNNEL_KEY_ATTR_PAD,
> __OVS_TUNNEL_KEY_ATTR_MAX
>  };

The above changes have not been added to upstream linux header file,
and the kernel uapi headers are the source of truth for these values.
Please define them in such a way that they only apply to userspace so
that we can prevent any potential conflict between expectations of
meaning of these enums. I see common ways to achieve this have been to
use `#ifndef __KERNEL__` conditional and by defining them as higher
numbers that are less likely to conflict like the existing tunnel
types.

> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> index 61a67de..fb41736 100644
> --- a/include/openvswitch/match.h
> +++ b/include/openvswitch/match.h
> @@ -109,6 +109,10 @@ void match_set_tun_gbp_id_masked(struct match *match, 
> ovs_be16 gbp_id, ovs_be16
>  void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
>  void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags, 
> uint8_t mask);
>  void match_set_tun_gbp_flags(struct match *match, uint8_t flags);
> +void match_set_tun_gtpu_flags(struct match *match, uint8_t gtpu_flags);
> +void match_set_tun_gtpu_flags_masked(struct match *match, uint8_t gtpu_flags,
> + uint8_t mask);
> +void match_set_tun_gtpu_msgtype(struct match *match, uint8_t gtpu_msgtype);
>  void match_set_in_port(struct match *, ofp_port_t 

Re: [ovs-dev] [PATCH] docs: fix the documentation of n-handler-threads and n-revalidator-threads

[Joe Stringer]

On 6 February 2018 at 06:44, Matteo Croce  wrote:
> Documentation for both n-handler-threads and n-revalidator-threads was
> wrong, the spawned threads are per port instead of per datapath.
>
> Update the docs according to vswitchd/ovs-vswitchd.8.in and code behaviour.
>
> Signed-off-by: Matteo Croce 
> ---

Hi Matteo,

The only reference to handler or revalidator threads in
vswitchd/ovs-vswitchd.8.in is documenting how many file descriptors
are opened, which is dependent upon 'n-handler-threads'. This
documentation in vswitch.xml, on the other hand, describes the number
of threads are spawned, not the number of file descriptors (which is
correct):

$ grep -C 4 -e handler -e revalidator vswitchd/ovs-vswitchd.8.in
\fBovs\-vswitchd\fR started through \fBovs\-ctl\fR(8) provides a limit of 65535
file descriptors.  The limits on the number of bridges and ports is decided by
the availability of file descriptors.  With the Linux kernel datapath, creation
of a single bridge consumes three file descriptors and adding a port consumes
"n-handler-threads" file descriptors per bridge port.  Performance will degrade
beyond 1,024 ports per bridge due to fixed hash table sizing.  Other platforms
may have different limitations.
.
.IP \(bu

joe@dellingr ~/git/ovs $ ovs-vsctl get Open_vSwitch . other-config
{n-handler-threads="1", n-revalidator-threads="1"}
joe@dellingr ~/git/ovs $ ovs-vsctl show
fa5dee31-6e87-4b53-a4ac-84b2849e4fe2
Bridge "br0"
Port "p1"
Interface "p1"
type: dummy
Port "p0"
Interface "p0"
type: dummy
Port "dummy0"
Interface "dummy0"
Port "br0"
Interface "br0"
type: internal
joe@dellingr ~/git/ovs $ ps H -o 'tid comm' $(pidof ovs-vswitchd)
  TID COMMAND
16960 ovs-vswitchd
17325 ct_clean1
17326 urcu2
17937 handler15
17938 revalidator16

Above, you can see that I set these two settings to 1, and added a
bunch of ports to OVS, then observed that there is still just one of
each of the threads. Therefore the setting is not per-port.

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

Re: [ovs-dev] [PATCH] odp-util: Fix buffer overread in parsing string form of ODP flows.

[Joe Stringer]

On 26 November 2017 at 17:41, Ben Pfaff <b...@ovn.org> wrote:
> scan_u128() should return 0 on an error but it actually returned an errno
> value in some cases, so a command like this:
> ovs-appctl dpctl/add-flow 'ct_label(1/55)' ''
> could cause a buffer overread.
>
> This bug is not as severe as it may sound because the string form of ODP
> flows is not used over OpenFlow or OVSDB, only through the appctl interface
> that is normally used just by local system administrators and not exposed
> over a network.
>
> Reported-by: Bhargava Shastry <bshas...@sec.t-labs.tu-berlin.de>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---

Acked-by: Joe Stringer <j...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Transition ukey on dp_ops error.

[Joe Stringer]

On Thu, 2 Nov 2017, 15:02 Ben Pfaff, <b...@ovn.org> wrote:

> On Mon, Sep 18, 2017 at 02:56:35PM -0700, Greg Rose wrote:
> > On 09/06/2017 03:12 PM, Joe Stringer wrote:
> > >In most situations, we don't expect that a flow we've successfully
> > >dumped, which we intend to delete, cannot be deleted. However, to make
> > >this code more resilient to ensure that ukeys *will* transition in all
> > >cases (including an error at this stage), grab the lock and transition
> > >this ukey forward to the evicted state, effectively treating a failure
> > >to delete as "this flow is already gone".
> > >
> > >If we subsequently find out that it wasn't deleted, then that's ok - we
> > >will re-dump, and validate at that stage, which should lead to creating
> > >a new ukey or deleting the datapath flow when that happens.
> > >
> > >Signed-off-by: Joe Stringer <j...@ovn.org>
> > >---
> > >  ofproto/ofproto-dpif-upcall.c | 5 +
> > >  1 file changed, 5 insertions(+)
> > >
> > >diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> > >index 4a71bbe258df..bd324fbb6323 100644
> > >--- a/ofproto/ofproto-dpif-upcall.c
> > >+++ b/ofproto/ofproto-dpif-upcall.c
> > >@@ -2227,6 +2227,11 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
> *ops, size_t n_ops)
> > >  if (op->dop.error) {
> > >  /* flow_del error, 'stats' is unusable. */
> > >+if (op->ukey) {
> > >+ovs_mutex_lock(>ukey->mutex);
> > >+transition_ukey(op->ukey, UKEY_EVICTED);
> > >+ovs_mutex_unlock(>ukey->mutex);
> > >+}
> > >  continue;
> > >  }
> > >
> >
> > Compile tested only - I didn't see of any good way to force the error
> >
> > Code looks good to me.
> >
> > Reviewed-by: Greg Rose <gvrose8...@gmail.com>
>
> I applied this to master, although I don't fully understand it so that
> seems like living dangerously.  That's EXTREME programming.
>

Here was I, thinking that extreme programming involved at least one of a
volcano, parachute, or snowboard...

Thanks for applying this.

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

Re: [ovs-dev] [PATCH net-next] openvswitch: add ct_clear action

[Joe Stringer]

On 10 October 2017 at 12:13, Eric Garver <e...@erig.me> wrote:
> On Tue, Oct 10, 2017 at 10:24:20AM -0700, Joe Stringer wrote:
>> On 10 October 2017 at 08:09, Eric Garver <e...@erig.me> wrote:
>> > On Tue, Oct 10, 2017 at 05:33:48AM -0700, Joe Stringer wrote:
>> >> On 9 October 2017 at 21:41, Pravin Shelar <pshe...@ovn.org> wrote:
>> >> > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e...@erig.me> wrote:
>> >> >> This adds a ct_clear action for clearing conntrack state. ct_clear is
>> >> >> currently implemented in OVS userspace, but is not backed by an action
>> >> >> in the kernel datapath. This is useful for flows that may modify a
>> >> >> packet tuple after a ct lookup has already occurred.
>> >> >>
>> >> >> Signed-off-by: Eric Garver <e...@erig.me>
>> >> > Patch mostly looks good. I have following comments.
>> >> >
>> >> >> ---
>> >> >>  include/uapi/linux/openvswitch.h |  2 ++
>> >> >>  net/openvswitch/actions.c|  5 +
>> >> >>  net/openvswitch/conntrack.c  | 12 
>> >> >>  net/openvswitch/conntrack.h  |  7 +++
>> >> >>  net/openvswitch/flow_netlink.c   |  5 +
>> >> >>  5 files changed, 31 insertions(+)
>> >> >>
>> >> >> diff --git a/include/uapi/linux/openvswitch.h 
>> >> >> b/include/uapi/linux/openvswitch.h
>> >> >> index 156ee4cab82e..1b6e510e2cc6 100644
>> >> >> --- a/include/uapi/linux/openvswitch.h
>> >> >> +++ b/include/uapi/linux/openvswitch.h
>> >> >> @@ -806,6 +806,7 @@ struct ovs_action_push_eth {
>> >> >>   * packet.
>> >> >>   * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the
>> >> >>   * packet.
>> >> >> + * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet.
>> >> >>   *
>> >> >>   * Only a single header can be set with a single 
>> >> >> %OVS_ACTION_ATTR_SET.  Not all
>> >> >>   * fields within a header are modifiable, e.g. the IPv4 protocol and 
>> >> >> fragment
>> >> >> @@ -835,6 +836,7 @@ enum ovs_action_attr {
>> >> >> OVS_ACTION_ATTR_TRUNC,/* u32 struct ovs_action_trunc. 
>> >> >> */
>> >> >> OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
>> >> >> OVS_ACTION_ATTR_POP_ETH,  /* No argument. */
>> >> >> +   OVS_ACTION_ATTR_CT_CLEAR, /* No argument. */
>> >> >>
>> >> >> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be 
>> >> >> accepted
>> >> >>* from userspace. */
>> >> >> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> >> >> index a54a556fcdb5..db9c7f2e662b 100644
>> >> >> --- a/net/openvswitch/actions.c
>> >> >> +++ b/net/openvswitch/actions.c
>> >> >> @@ -1203,6 +1203,10 @@ static int do_execute_actions(struct datapath 
>> >> >> *dp, struct sk_buff *skb,
>> >> >> return err == -EINPROGRESS ? 0 : err;
>> >> >> break;
>> >> >>
>> >> >> +   case OVS_ACTION_ATTR_CT_CLEAR:
>> >> >> +   err = ovs_ct_clear(skb, key);
>> >> >> +   break;
>> >> >> +
>> >> >> case OVS_ACTION_ATTR_PUSH_ETH:
>> >> >> err = push_eth(skb, key, nla_data(a));
>> >> >> break;
>> >> >> @@ -1210,6 +1214,7 @@ static int do_execute_actions(struct datapath 
>> >> >> *dp, struct sk_buff *skb,
>> >> >> case OVS_ACTION_ATTR_POP_ETH:
>> >> >> err = pop_eth(skb, key);
>> >> >> break;
>> >> >> +
>> >> >> }
>> >> > Unrelated change.
>> >> >
>> >> >>
>> >> >> if (unlikely(err)) {
>> >> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> 

Re: [ovs-dev] [PATCH net-next] openvswitch: add ct_clear action

[Joe Stringer]

On 10 October 2017 at 08:09, Eric Garver <e...@erig.me> wrote:
> On Tue, Oct 10, 2017 at 05:33:48AM -0700, Joe Stringer wrote:
>> On 9 October 2017 at 21:41, Pravin Shelar <pshe...@ovn.org> wrote:
>> > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e...@erig.me> wrote:
>> >> This adds a ct_clear action for clearing conntrack state. ct_clear is
>> >> currently implemented in OVS userspace, but is not backed by an action
>> >> in the kernel datapath. This is useful for flows that may modify a
>> >> packet tuple after a ct lookup has already occurred.
>> >>
>> >> Signed-off-by: Eric Garver <e...@erig.me>
>> > Patch mostly looks good. I have following comments.
>> >
>> >> ---
>> >>  include/uapi/linux/openvswitch.h |  2 ++
>> >>  net/openvswitch/actions.c|  5 +
>> >>  net/openvswitch/conntrack.c  | 12 
>> >>  net/openvswitch/conntrack.h  |  7 +++
>> >>  net/openvswitch/flow_netlink.c   |  5 +
>> >>  5 files changed, 31 insertions(+)
>> >>
>> >> diff --git a/include/uapi/linux/openvswitch.h 
>> >> b/include/uapi/linux/openvswitch.h
>> >> index 156ee4cab82e..1b6e510e2cc6 100644
>> >> --- a/include/uapi/linux/openvswitch.h
>> >> +++ b/include/uapi/linux/openvswitch.h
>> >> @@ -806,6 +806,7 @@ struct ovs_action_push_eth {
>> >>   * packet.
>> >>   * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the
>> >>   * packet.
>> >> + * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet.
>> >>   *
>> >>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  
>> >> Not all
>> >>   * fields within a header are modifiable, e.g. the IPv4 protocol and 
>> >> fragment
>> >> @@ -835,6 +836,7 @@ enum ovs_action_attr {
>> >> OVS_ACTION_ATTR_TRUNC,/* u32 struct ovs_action_trunc. */
>> >> OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
>> >> OVS_ACTION_ATTR_POP_ETH,  /* No argument. */
>> >> +   OVS_ACTION_ATTR_CT_CLEAR, /* No argument. */
>> >>
>> >> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be 
>> >> accepted
>> >>* from userspace. */
>> >> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> >> index a54a556fcdb5..db9c7f2e662b 100644
>> >> --- a/net/openvswitch/actions.c
>> >> +++ b/net/openvswitch/actions.c
>> >> @@ -1203,6 +1203,10 @@ static int do_execute_actions(struct datapath *dp, 
>> >> struct sk_buff *skb,
>> >> return err == -EINPROGRESS ? 0 : err;
>> >> break;
>> >>
>> >> +   case OVS_ACTION_ATTR_CT_CLEAR:
>> >> +   err = ovs_ct_clear(skb, key);
>> >> +   break;
>> >> +
>> >> case OVS_ACTION_ATTR_PUSH_ETH:
>> >> err = push_eth(skb, key, nla_data(a));
>> >> break;
>> >> @@ -1210,6 +1214,7 @@ static int do_execute_actions(struct datapath *dp, 
>> >> struct sk_buff *skb,
>> >> case OVS_ACTION_ATTR_POP_ETH:
>> >> err = pop_eth(skb, key);
>> >> break;
>> >> +
>> >> }
>> > Unrelated change.
>> >
>> >>
>> >> if (unlikely(err)) {
>> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> >> index d558e882ca0c..f9b73c726ad7 100644
>> >> --- a/net/openvswitch/conntrack.c
>> >> +++ b/net/openvswitch/conntrack.c
>> >> @@ -1129,6 +1129,18 @@ int ovs_ct_execute(struct net *net, struct sk_buff 
>> >> *skb,
>> >> return err;
>> >>  }
>> >>
>> >> +int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
>> >> +{
>> >> +   if (skb_nfct(skb)) {
>> >> +   nf_conntrack_put(skb_nfct(skb));
>> >> +   nf_ct_set(skb, NULL, 0);
>> > Can the new conntract state be appropriate? may be IP_CT_UNTRACKED?
>> >
>> >> +   }
>> >> +
>> >> +   ovs_ct_fill_key(skb, key);
>> >> +
>> > I do not see need to refill the key if there is no skb-nf-ct.
>>
>> Really this is trying to just zero the CT key fields, but reuses
>> existing functions, right? This means that subsequent upcalls, for
>
> Right.
>
>> instance, won't have the outdated view of the CT state from the
>> previous lookup (that was prior to the ct_clear). I'd expect these key
>> fields to be cleared.
>
> I assumed Pravin was saying that we don't need to clear them if there is
> no conntrack state. They should already be zero.

The conntrack calls aren't going to clear it, so I don't see what else
would clear it?

If you execute ct(),ct_clear(), then the first ct will set the
values.. what will zero them?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next] openvswitch: add ct_clear action

[Joe Stringer]

On 9 October 2017 at 21:41, Pravin Shelar  wrote:
> On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver  wrote:
>> This adds a ct_clear action for clearing conntrack state. ct_clear is
>> currently implemented in OVS userspace, but is not backed by an action
>> in the kernel datapath. This is useful for flows that may modify a
>> packet tuple after a ct lookup has already occurred.
>>
>> Signed-off-by: Eric Garver 
> Patch mostly looks good. I have following comments.
>
>> ---
>>  include/uapi/linux/openvswitch.h |  2 ++
>>  net/openvswitch/actions.c|  5 +
>>  net/openvswitch/conntrack.c  | 12 
>>  net/openvswitch/conntrack.h  |  7 +++
>>  net/openvswitch/flow_netlink.c   |  5 +
>>  5 files changed, 31 insertions(+)
>>
>> diff --git a/include/uapi/linux/openvswitch.h 
>> b/include/uapi/linux/openvswitch.h
>> index 156ee4cab82e..1b6e510e2cc6 100644
>> --- a/include/uapi/linux/openvswitch.h
>> +++ b/include/uapi/linux/openvswitch.h
>> @@ -806,6 +806,7 @@ struct ovs_action_push_eth {
>>   * packet.
>>   * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the
>>   * packet.
>> + * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet.
>>   *
>>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not 
>> all
>>   * fields within a header are modifiable, e.g. the IPv4 protocol and 
>> fragment
>> @@ -835,6 +836,7 @@ enum ovs_action_attr {
>> OVS_ACTION_ATTR_TRUNC,/* u32 struct ovs_action_trunc. */
>> OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
>> OVS_ACTION_ATTR_POP_ETH,  /* No argument. */
>> +   OVS_ACTION_ATTR_CT_CLEAR, /* No argument. */
>>
>> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>>* from userspace. */
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index a54a556fcdb5..db9c7f2e662b 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -1203,6 +1203,10 @@ static int do_execute_actions(struct datapath *dp, 
>> struct sk_buff *skb,
>> return err == -EINPROGRESS ? 0 : err;
>> break;
>>
>> +   case OVS_ACTION_ATTR_CT_CLEAR:
>> +   err = ovs_ct_clear(skb, key);
>> +   break;
>> +
>> case OVS_ACTION_ATTR_PUSH_ETH:
>> err = push_eth(skb, key, nla_data(a));
>> break;
>> @@ -1210,6 +1214,7 @@ static int do_execute_actions(struct datapath *dp, 
>> struct sk_buff *skb,
>> case OVS_ACTION_ATTR_POP_ETH:
>> err = pop_eth(skb, key);
>> break;
>> +
>> }
> Unrelated change.
>
>>
>> if (unlikely(err)) {
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index d558e882ca0c..f9b73c726ad7 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -1129,6 +1129,18 @@ int ovs_ct_execute(struct net *net, struct sk_buff 
>> *skb,
>> return err;
>>  }
>>
>> +int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
>> +{
>> +   if (skb_nfct(skb)) {
>> +   nf_conntrack_put(skb_nfct(skb));
>> +   nf_ct_set(skb, NULL, 0);
> Can the new conntract state be appropriate? may be IP_CT_UNTRACKED?
>
>> +   }
>> +
>> +   ovs_ct_fill_key(skb, key);
>> +
> I do not see need to refill the key if there is no skb-nf-ct.

Really this is trying to just zero the CT key fields, but reuses
existing functions, right? This means that subsequent upcalls, for
instance, won't have the outdated view of the CT state from the
previous lookup (that was prior to the ct_clear). I'd expect these key
fields to be cleared.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netdev: Use portable error code for zero rate meter band

[Joe Stringer]

On 28 September 2017 at 12:39, Andy Zhou <az...@ovn.org> wrote:
> 'EBADRQC' is only defined on the Linux platform. Without this fix,
> The travis MacOS build fails. Switching to using EDOM which is more
> portable.
>
> Fixes: 2029ce9ac3a601 (dpif-netdev: Fix a zero-rate bug for meter)
> CC: Ali Volkan ATLI <volkan.a...@argela.com.tr>
> Signed-off-by: Andy Zhou <az...@ovn.org>

LGTM.

Acked-by: Joe Stringer <j...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [v2] bridge: Fix controller status update to passive connections

[Joe Stringer]

On 13 September 2017 at 13:05, Andy Zhou <az...@ovn.org> wrote:
> The bug can cause ovs-vswitchd to crash (due to assert) when it is
> set up with a passive controller connection. Since only active
> connections are kept, the passive connection status update should be
> ignored and not trigger asserts.
>
> Fixes: 85c55772a453 ("bridge: Fix controller status update")
> Reported-by: Josh Bailey <j...@faucet.nz>
> Signed-off-by: Andy Zhou <az...@ovn.org>

Acked-by: Joe Stringer <j...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] bridge: Fix controller status update to passive connections

[Joe Stringer]

On 6 September 2017 at 15:24, Andy Zhou  wrote:
> The bug can cause ovs-vswitchd to crash (due to assert) when it is
> set up with a passive controller connection. Since only active
> connections are kept, the passive connection status update should be
> ignored and not trigger asserts.
>

Fixes: 85c55772a453 ("bridge: Fix controller status update")

> Reported-by: Josh Bailey 
> Signed-off-by: Andy Zhou 
> ---

This bug was reported on v2.8.0, so will need backport to branch-2.8.

>  AUTHORS.rst   |  1 +
>  vswitchd/bridge.c | 12 +++-
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/AUTHORS.rst b/AUTHORS.rst
> index cb0cd91b21ba..e304609b2b27 100644
> --- a/AUTHORS.rst
> +++ b/AUTHORS.rst
> @@ -389,6 +389,7 @@ Ben Basler  bbas...@nicira.com
>  Bhargava Shastrybshas...@sec.t-labs.tu-berlin.de
>  Bob Ballbob.b...@citrix.com
>  Brad Hall   b...@nicira.com
> +Brailey Joshj...@faucet.nz
>  Brandon Heller  brand...@stanford.edu
>  Brendan Kelley  bkel...@nicira.com
>  Brent Salisbury brent.salisb...@gmail.com
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index a8cbae78cb23..00f86182c820 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2720,11 +2720,13 @@ refresh_controller_status(void)
>  struct ofproto_controller_info *cinfo =
>  shash_find_data(, cfg->target);
>
> -ovs_assert(cinfo);
> -ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
> -const char *role = ofp12_controller_role_to_str(cinfo->role);
> -ovsrec_controller_set_role(cfg, role);
> -ovsrec_controller_set_status(cfg, >pairs);
> +/* cinfo is NULL when 'cfg->target' is a passive connection.  */
> +if (cinfo) {
> +ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
> +const char *role = ofp12_controller_role_to_str(cinfo->role);
> +ovsrec_controller_set_role(cfg, role);
> +ovsrec_controller_set_status(cfg, >pairs);
> +}

Prior to commit 85c55772a453f, the following lines were in the alternative case:
ovsrec_controller_set_is_connected(cfg, false);
ovsrec_controller_set_role(cfg, NULL);
ovsrec_controller_set_status(cfg, NULL);

Should we update the records in the else case here like was previously done?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ofproto-dpif-upcall: Transition ukey on dp_ops error.

[Joe Stringer]

In most situations, we don't expect that a flow we've successfully
dumped, which we intend to delete, cannot be deleted. However, to make
this code more resilient to ensure that ukeys *will* transition in all
cases (including an error at this stage), grab the lock and transition
this ukey forward to the evicted state, effectively treating a failure
to delete as "this flow is already gone".

If we subsequently find out that it wasn't deleted, then that's ok - we
will re-dump, and validate at that stage, which should lead to creating
a new ukey or deleting the datapath flow when that happens.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
 ofproto/ofproto-dpif-upcall.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 4a71bbe258df..bd324fbb6323 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2227,6 +2227,11 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, 
size_t n_ops)
 
 if (op->dop.error) {
 /* flow_del error, 'stats' is unusable. */
+if (op->ukey) {
+ovs_mutex_lock(>ukey->mutex);
+transition_ukey(op->ukey, UKEY_EVICTED);
+ovs_mutex_unlock(>ukey->mutex);
+}
 continue;
 }
 
-- 
2.14.1

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

Re: [ovs-dev] [PATCH] docs: Add references to git-pw

[Joe Stringer]

On 31 August 2017 at 02:02, Stephen Finucane <step...@that.guru> wrote:
> On Thu, 2017-08-31 at 09:59 +0100, Stephen Finucane wrote:
>> On Wed, 2017-08-30 at 10:38 -0700, Joe Stringer wrote:
>> > On 29 August 2017 at 02:54, Stephen Finucane <step...@that.guru> wrote:
>
> [snip]
>
>> > However, it seems like this is broken right now. (git-pw patch list
>> > returns empty list if I have project configured)
>>
>> Eek. So that's a bug with Patchwork, not git-pw. I've posted the patch [1]
>> and it should be there before the end of the week. Good catch.
>
> If you did want to play with this rn, using the project name (as opposed to 
> the
> linkname) will let you do so:
>
>   git config pw.project 'open vswitch'

Ah, nice. Funny how assumptions like this can leak into the code :-)

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

Re: [ovs-dev] [PATCH 1/2] datapath: compat: Fix build on RHEL 7.4

[Joe Stringer]

On 22 August 2017 at 18:00, Joe Stringer <j...@ovn.org> wrote:
> On 22 August 2017 at 17:52, Yi-Hung Wei <yihung@gmail.com> wrote:
>> RHEL 7.4 introduces netdev_master_upper_dev_link_rh() that breaks the
>> backport of OVS kernel module on RHEL 7.4. This patch fixes that issue.
>>
>> Signed-off-by: Yi-Hung Wei <yihung@gmail.com>
>> ---
>
> Thanks YiHung, LGTM.
>
> Unless others have objections, it seems like this would be trivial to
> also backport to branch-2.8 and allow that release to compile against
> CentOS/RHEL 7.4 kernels.

Applied to master and branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] how to run script in *.at file in tests folder?

[Joe Stringer]

On 23 August 2017 at 10:32, Joe Stringer <j...@ovn.org> wrote:
> On 23 August 2017 at 01:04, Sam <batmanu...@gmail.com> wrote:
>> Hi all,
>>
>> I'm testing, I add a new at file, and in that file, I run another shell
>> script like this:
>>
>> AT_SETUP([vhost - run prepare-env.sh])
>>> #OVS_VSWITCHD_START
>>> # No need to create, as bond1 has been created.
>>> echo "@"
>>> /root/gangyewei/mvs/mvs/tests/prepare-env.sh
>>> AT_CHECK([echo ""])
>>> AT_CHECK([/root/gangyewei/mvs/mvs/tests/prepare_env], [0], [stdout])
>>> #OVS_VSWITCHD_STOP
>>> AT_CLEANUP
>>
>>
>> I use root to run the tests, But the log show me permission error, how to
>> fix this bug and how to run shell script in at file?
>>
>> # -*- compilation -*-
>>> 1. vhost.at:23: testing vhost - run prepare-env.sh ...
>>> @
>>> /root/gangyewei/mvs/mvs/tests/testsuite.dir/at-groups/1/test-source: line
>>> 12: /root/gangyewei/mvs/mvs/tests/prepare-env.sh: Permission denied
>>> ./vhost.at:30: echo ""
>
> The above is the command being run.
>
>>> --- /dev/null   2017-02-21 23:39:22.88249 +0800
>>> +++ /root/gangyewei/mvs/mvs/tests/testsuite.dir/at-groups/1/stdout
>>>  2017-08-23 15:43:29.348993538 +0800
>>> @@ -0,0 +1 @@
>>> +
>
> The above is intended to read like a diff from expected to actual
> results, and it states that the line with "" is extra
> compared to the expected results.
>
> AT_CHECK by default will check that the command is successful, and
> also check that there is no output.
>
> Perhaps try something like this?
> AT_CHECK([echo ""], [0], [ignore])

Ah, I thought you weren't getting a chance to execute because the test
didn't proceed beyond this point. Perhaps you should check the execute
permissions on the script.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] how to run script in *.at file in tests folder?

[Joe Stringer]

On 23 August 2017 at 01:04, Sam  wrote:
> Hi all,
>
> I'm testing, I add a new at file, and in that file, I run another shell
> script like this:
>
> AT_SETUP([vhost - run prepare-env.sh])
>> #OVS_VSWITCHD_START
>> # No need to create, as bond1 has been created.
>> echo "@"
>> /root/gangyewei/mvs/mvs/tests/prepare-env.sh
>> AT_CHECK([echo ""])
>> AT_CHECK([/root/gangyewei/mvs/mvs/tests/prepare_env], [0], [stdout])
>> #OVS_VSWITCHD_STOP
>> AT_CLEANUP
>
>
> I use root to run the tests, But the log show me permission error, how to
> fix this bug and how to run shell script in at file?
>
> # -*- compilation -*-
>> 1. vhost.at:23: testing vhost - run prepare-env.sh ...
>> @
>> /root/gangyewei/mvs/mvs/tests/testsuite.dir/at-groups/1/test-source: line
>> 12: /root/gangyewei/mvs/mvs/tests/prepare-env.sh: Permission denied
>> ./vhost.at:30: echo ""

The above is the command being run.

>> --- /dev/null   2017-02-21 23:39:22.88249 +0800
>> +++ /root/gangyewei/mvs/mvs/tests/testsuite.dir/at-groups/1/stdout
>>  2017-08-23 15:43:29.348993538 +0800
>> @@ -0,0 +1 @@
>> +

The above is intended to read like a diff from expected to actual
results, and it states that the line with "" is extra
compared to the expected results.

AT_CHECK by default will check that the command is successful, and
also check that there is no output.

Perhaps try something like this?
AT_CHECK([echo ""], [0], [ignore])
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] datapath: compat: Fix build on RHEL 7.4

[Joe Stringer]

On 22 August 2017 at 17:52, Yi-Hung Wei  wrote:
> RHEL 7.4 introduces netdev_master_upper_dev_link_rh() that breaks the
> backport of OVS kernel module on RHEL 7.4. This patch fixes that issue.
>
> Signed-off-by: Yi-Hung Wei 
> ---

Thanks YiHung, LGTM.

Unless others have objections, it seems like this would be trivial to
also backport to branch-2.8 and allow that release to compile against
CentOS/RHEL 7.4 kernels.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/4] tc: Add header rewrite using tc pedit action

[Joe Stringer]

On 22 August 2017 at 13:31, Paul Blakey <pa...@mellanox.com> wrote:
>
>
> On 22/08/2017 23:07, Paul Blakey wrote:
>>
>>
>>
>> On 21/08/2017 20:45, Joe Stringer wrote:
>>>
>>> On 20 August 2017 at 01:50, Paul Blakey <pa...@mellanox.com> wrote:
>>>>
>>>>
>>>>
>>>> On 18/08/2017 00:52, Joe Stringer wrote:
>>>>>
>>>>>
>>>>> On 17 August 2017 at 02:18, Paul Blakey <pa...@mellanox.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 15/08/2017 20:04, Joe Stringer wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 15 August 2017 at 01:19, Paul Blakey <pa...@mellanox.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 15/08/2017 00:56, Joe Stringer wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8 August 2017 at 07:21, Roi Dayan <r...@mellanox.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> From: Paul Blakey <pa...@mellanox.com>
>>>>>>>>>>
>>>>>>>>>> @@ -117,6 +118,17 @@ struct tc_flower {
>>>>>>>>>>  uint64_t lastused;
>>>>>>>>>>
>>>>>>>>>>  struct {
>>>>>>>>>> +bool rewrite;
>>>>>>>>>> +uint8_t pad1[3];
>>>>>>>>>> +struct tc_flower_key key;
>>>>>>>>>> +uint8_t pad2[3];
>>>>>>>>>> +struct tc_flower_key mask;
>>>>>>>>>> +uint8_t pad3[3];
>>>>>>>>>> +} rewrite;
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Now that I get why the pads are here.. ;)
>>>>>>>
>>>>>>> Is there an existing macro we can use to ensure that these pad out to
>>>>>>> 32-bit boundaries?
>>>>>>>
>>>>>>
>>>>>> I'm not sure if that's possible, the size is a minimum of extra
>>>>>> 24bits,
>>>>>> so
>>>>>> its can't overflow with writing on any address below it. The compiler
>>>>>> might add some extra padding but that shouldn't matter.
>>>>>
>>>>>
>>>>>
>>>>> My thought was that the main concern here is to align the fields to
>>>>> 32-bit boundaries, so if it already does this then I wouldn't expect
>>>>> to need any padding at all? For instance, on my system with these
>>>>> patches I see with pahole that 'struct tc_flower_key' has size 84, and
>>>>> the 'pad2' and 'pad3' appear to be unnecessary:
>>>>>
>>>>>   struct {
>>>>>  _Bool  rewrite;  /*   216
>>>>> 1 */
>>>>>  uint8_tpad1[0];  /*   217
>>>>> 0 */
>>>>>  struct tc_flower_key key;/*   220
>>>>> 84 */
>>>>>  /* --- cacheline 1 boundary (64 bytes) was 21 bytes
>>>>> ago
>>>>> --- */
>>>>>  uint8_tpad2[0];  /*   304
>>>>> 0 */
>>>>>  struct tc_flower_key mask;   /*   308
>>>>> 84 */
>>>>>  /* --- cacheline 2 boundary (128 bytes) was 41 bytes
>>>>> ago
>>>>> --- */
>>>>>  uint8_tpad3[0];  /*   392
>>>>> 0 */
>>>>>  } rewrite;   /*   216
>>>>> 180 */
>>>>>
>>>>> However, if in future someone adds a 1-byte member to tc_flower_key
>>>>> then I'm not sure that this alignment will hold. On my system, 

Re: [ovs-dev] [PATCH] datapath: fix skb_panic due to the incorrect actions attrlen

[Joe Stringer]

On 22 August 2017 at 08:26, Greg Rose <gvrose8...@gmail.com> wrote:
> On 08/18/2017 02:41 PM, Joe Stringer wrote:
>>
>> On 16 August 2017 at 15:48, Greg Rose <gvrose8...@gmail.com> wrote:
>> > Upstream commit:
>> >  commit 494bea39f3201776cdfddc232705f54a0bd210c4
>> >  Author: Liping Zhang <zlpnob...@gmail.com>
>> >  Date:   Wed Aug 16 13:30:07 2017 +0800
>> >
>> >  For sw_flow_actions, the actions_len only represents the kernel
>> > part's
>> >  size, and when we dump the actions to the userspace, we will do the
>> >  convertions, so it's true size may become bigger than the
>> > actions_len.
>> >
>> >  But unfortunately, for OVS_PACKET_ATTR_ACTIONS, we use the
>> > actions_len
>> >  to alloc the skbuff, so the user_skb's size may become insufficient
>> > and
>> >  oops will happen like this:
>> >skbuff: skb_over_panic: text:8148fabf len:1749 put:157
>> > head:
>> >881300f39000 data:881300f39000 tail:0x6d5 end:0x6c0
>> > dev:
>> >[ cut here ]
>> >kernel BUG at net/core/skbuff.c:129!
>> >[...]
>> >Call Trace:
>> > 
>> > [] skb_put+0x43/0x44
>> > [] skb_zerocopy+0x6c/0x1f4
>> > [] queue_userspace_packet+0x3a3/0x448
>> > [openvswitch]
>> > [] ovs_dp_upcall+0x30/0x5c [openvswitch]
>> > [] output_userspace+0x132/0x158 [openvswitch]
>> > [] ? ip6_rcv_finish+0x74/0x77 [ipv6]
>> > [] do_execute_actions+0xcc1/0xdc8
>> > [openvswitch]
>> > [] ovs_execute_actions+0x74/0x106
>> > [openvswitch]
>> > [] ovs_dp_process_packet+0xe1/0xfd
>> > [openvswitch]
>> > [] ? key_extract+0x63c/0x8d5 [openvswitch]
>> > [] ovs_vport_receive+0xa1/0xc3 [openvswitch]
>> >[...]
>> >
>> >  Also we can find that the actions_len is much little than the
>> > orig_len:
>> >crash> struct sw_flow_actions 0x8812f539d000
>> >struct sw_flow_actions {
>> >  rcu = {
>> >next = 0x8812f5398800,
>> >func = 0xe3b00035db32
>> >  },
>> >  orig_len = 1384,
>> >  actions_len = 592,
>> >  actions = 0x8812f539d01c
>> >}
>> >
>> >  So as a quick fix, use the orig_len instead of the actions_len to
>> > alloc
>> >  the user_skb.
>> >
>> >  Last, this oops happened on our system running a relative old
>> > kernel, but
>> >  the same risk still exists on the mainline, since we use the wrong
>> >  actions_len from the beginning.
>> >
>> >  Fixes: 0e469d3b380c ("datapath: include datapath actions with
>> > sampled-"...)
>> >  Cc: Neil McKee <neil.mc...@inmon.com>
>> >  Signed-off-by: Liping Zhang <zlpnob...@gmail.com>
>> >  Acked-by: Pravin B Shelar <pshe...@ovn.org>
>> >  Signed-off-by: David S. Miller <da...@davemloft.net>
>> >
>> > Signed-off-by: Greg Rose <gvrose8...@gmail.com>
>> > ---
>>
>> Thanks for the backport.
>>
>> It seems like we're a bit diverged from the latest net-next, if I
>> follow correctly these patches haven't been backported yet. Would you
>> mind preparing these, too?
>>
>> 0ed80da518a1 openvswitch: Remove unnecessary newlines from OVS_NLERR uses
>> c4b2bf6b4a35 openvswitch: Optimize operations for OvS flow_stats.
>> c57c054eb5b1 openvswitch: Optimize updating for OvS flow_stats.
>> 880388aa3c07 net: Remove all references to SKB_GSO_UDP.
>>
>> I realise that this particular patch is a long-standing bug that we
>> can fix (and will need backporting in our tree), but it's nice if we
>> can keep the patches applied to master here in the same order as
>> upstream net-next so that it's easier to tell how out of sync we are
>> by a side-by-side comparison of "git log --oneline" for datapath/ or
>> net/openvswitch in the corresponding trees. I also realise that this
>> patch didn't land in net-next yet since it's so new, but it's fairly
>> safe to assume it'll be applied there next.
>>
>> Cheers,
>> Joe
>>
> OK - I had been waiting for patches to hit the net tree before backporting.
> I'll get the ones in net-next too and submit a patch series.

Thanks! I'll keep an eye out for them.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2] ofproto-dpif: Mark packets as "untracked" after call to ct().

[Joe Stringer]

On 10 August 2017 at 23:38, Justin Pettit <jpet...@ovn.org> wrote:
> Packet and Connection state is only available to the processing path
> that follows the "recirc_table" argument of the ct() action.  The
> previous behavior made these states available until the end of the
> pipeline.  This commit changes the behavior so that the Packet and
> Connection state are cleared for the current processing path whenever
> ct() is called (in addition to reaching the end of the pipeline.)
>
> A future commit will remove the behavior that a "send to controller"
> action causes all packets for that flow to be handled via the slow-path.
> The current behavior of connection tracking state makes that difficult
> due to datapath actions containing multiple OpenFlow rules that may
> contain different connection tracking states.  This change will make
> that future commit possible.
>
> Signed-off-by: Justin Pettit <jpet...@ovn.org>
> ---

Overall I think that this is an improvement to consistency of what
connection tracking metadata is accessible from different points in
the OpenFlow pipeline. Although this will restrict the availability of
ct_state following the ct() action execution, controller writers who
wish to preserve access to this content across a CT action execution
can do so using registers. In practice I'm not aware of any controller
that is currently operating this way though.

Do we need a NEWS item for this?

Acked-by: Joe Stringer <j...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2] checkpatch: Enforce bracing around conditionals.

[Joe Stringer]

On 21 August 2017 at 06:31, Aaron Conole <acon...@redhat.com> wrote:
> Joe Stringer <j...@ovn.org> writes:
>
>> The coding style states that BSD-style brace placement should be used,
>> and even single statements should be enclosed. Add checks to checkpatch
>> for this, particularly for 'else' statements.
>>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>
> Acked-by: Aaron Conole <acon...@redhat.com>

Thanks, applied to master.

> Interestingly - if I do:
>
>   $ find lib/ -name \*.c -exec ./utilities/checkpatch.py -f {} \; | \
>  grep bracing | wc -l
>
> before this patch: 92 instances of 'Inappropriate bracing'
> after this patch: 102 instances of 'Inappropriate bracing'

Looks like a few may have slipped through the gaps in the pats. I
guess checkpatch.py is new enough that we haven't swept through the
repo and fixed the complaints yet.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/4] tc: Add header rewrite using tc pedit action

[Joe Stringer]

On 20 August 2017 at 01:50, Paul Blakey <pa...@mellanox.com> wrote:
>
>
> On 18/08/2017 00:52, Joe Stringer wrote:
>>
>> On 17 August 2017 at 02:18, Paul Blakey <pa...@mellanox.com> wrote:
>>>
>>>
>>>
>>> On 15/08/2017 20:04, Joe Stringer wrote:
>>>>
>>>>
>>>> On 15 August 2017 at 01:19, Paul Blakey <pa...@mellanox.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 15/08/2017 00:56, Joe Stringer wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8 August 2017 at 07:21, Roi Dayan <r...@mellanox.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: Paul Blakey <pa...@mellanox.com>
>>>>>>>
>>>>>>> @@ -117,6 +118,17 @@ struct tc_flower {
>>>>>>> uint64_t lastused;
>>>>>>>
>>>>>>> struct {
>>>>>>> +bool rewrite;
>>>>>>> +uint8_t pad1[3];
>>>>>>> +struct tc_flower_key key;
>>>>>>> +uint8_t pad2[3];
>>>>>>> +struct tc_flower_key mask;
>>>>>>> +uint8_t pad3[3];
>>>>>>> +} rewrite;
>>>>
>>>>
>>>>
>>>> Now that I get why the pads are here.. ;)
>>>>
>>>> Is there an existing macro we can use to ensure that these pad out to
>>>> 32-bit boundaries?
>>>>
>>>
>>> I'm not sure if that's possible, the size is a minimum of extra 24bits,
>>> so
>>> its can't overflow with writing on any address below it. The compiler
>>> might add some extra padding but that shouldn't matter.
>>
>>
>> My thought was that the main concern here is to align the fields to
>> 32-bit boundaries, so if it already does this then I wouldn't expect
>> to need any padding at all? For instance, on my system with these
>> patches I see with pahole that 'struct tc_flower_key' has size 84, and
>> the 'pad2' and 'pad3' appear to be unnecessary:
>>
>>  struct {
>> _Bool  rewrite;  /*   216 1 */
>> uint8_tpad1[0];  /*   217 0 */
>> struct tc_flower_key key;/*   22084 */
>> /* --- cacheline 1 boundary (64 bytes) was 21 bytes ago
>> --- */
>> uint8_tpad2[0];  /*   304 0 */
>> struct tc_flower_key mask;   /*   30884 */
>> /* --- cacheline 2 boundary (128 bytes) was 41 bytes ago
>> --- */
>> uint8_tpad3[0];  /*   392 0 */
>> } rewrite;   /*   216   180 */
>>
>> However, if in future someone adds a 1-byte member to tc_flower_key
>> then I'm not sure that this alignment will hold. On my system, it
>> seems like that would end up padding tc_flower_key out to 88B so these
>> extra padding would still be unnecessary (although pahole says that it
>> has a brain fart, so I'm not sure exactly how much confidence I should
>> have in this).
>>
>> What I was thinking about was whether we could use something like
>> PADDED_MEMBERS() in this case.
>>
>
> Are you talking about alignment in regards to performance?
> Because the padding is there for overflowing.
> If someone adds a new member to struct key/mask that is smaller than a
> 32bits to the end of the struct, setting it via a pointer might overflow the
> struct. I don't think PADDED_MEMBERS will work for this type of padding.
>
> We do mask the write to the write size, and it should still be in memory of
> owned by struct flower and since the compiler can't reorder the struct we
> actually only need the last padding to cover the above case.
>
> Maybe we can add alignment when we measure the performance of the code?

Ah. I wasn't concerned about performance, I was just wondering if this
forces us to add a few extra unnecessary bytes to 'rewrite' regardless
of the size of 'struct tc_flower'. For instance, if 'struct tc_flower'
is 63B long because someone adds a small field to the end of the
structure, then I'd expect to need only one extra byte of padding. If
it were a multiple of 32 bits, I wouldn't expect any need for padding
at all.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: fix skb_panic due to the incorrect actions attrlen

[Joe Stringer]

On 16 August 2017 at 15:48, Greg Rose  wrote:
> Upstream commit:
> commit 494bea39f3201776cdfddc232705f54a0bd210c4
> Author: Liping Zhang 
> Date:   Wed Aug 16 13:30:07 2017 +0800
>
> For sw_flow_actions, the actions_len only represents the kernel part's
> size, and when we dump the actions to the userspace, we will do the
> convertions, so it's true size may become bigger than the actions_len.
>
> But unfortunately, for OVS_PACKET_ATTR_ACTIONS, we use the actions_len
> to alloc the skbuff, so the user_skb's size may become insufficient and
> oops will happen like this:
>   skbuff: skb_over_panic: text:8148fabf len:1749 put:157 head:
>   881300f39000 data:881300f39000 tail:0x6d5 end:0x6c0 dev:
>   [ cut here ]
>   kernel BUG at net/core/skbuff.c:129!
>   [...]
>   Call Trace:
>
>[] skb_put+0x43/0x44
>[] skb_zerocopy+0x6c/0x1f4
>[] queue_userspace_packet+0x3a3/0x448 [openvswitch]
>[] ovs_dp_upcall+0x30/0x5c [openvswitch]
>[] output_userspace+0x132/0x158 [openvswitch]
>[] ? ip6_rcv_finish+0x74/0x77 [ipv6]
>[] do_execute_actions+0xcc1/0xdc8 [openvswitch]
>[] ovs_execute_actions+0x74/0x106 [openvswitch]
>[] ovs_dp_process_packet+0xe1/0xfd [openvswitch]
>[] ? key_extract+0x63c/0x8d5 [openvswitch]
>[] ovs_vport_receive+0xa1/0xc3 [openvswitch]
>   [...]
>
> Also we can find that the actions_len is much little than the orig_len:
>   crash> struct sw_flow_actions 0x8812f539d000
>   struct sw_flow_actions {
> rcu = {
>   next = 0x8812f5398800,
>   func = 0xe3b00035db32
> },
> orig_len = 1384,
> actions_len = 592,
> actions = 0x8812f539d01c
>   }
>
> So as a quick fix, use the orig_len instead of the actions_len to alloc
> the user_skb.
>
> Last, this oops happened on our system running a relative old kernel, but
> the same risk still exists on the mainline, since we use the wrong
> actions_len from the beginning.
>
> Fixes: 0e469d3b380c ("datapath: include datapath actions with 
> sampled-"...)
> Cc: Neil McKee 
> Signed-off-by: Liping Zhang 
> Acked-by: Pravin B Shelar 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Greg Rose 
> ---

Thanks for the backport.

It seems like we're a bit diverged from the latest net-next, if I
follow correctly these patches haven't been backported yet. Would you
mind preparing these, too?

0ed80da518a1 openvswitch: Remove unnecessary newlines from OVS_NLERR uses
c4b2bf6b4a35 openvswitch: Optimize operations for OvS flow_stats.
c57c054eb5b1 openvswitch: Optimize updating for OvS flow_stats.
880388aa3c07 net: Remove all references to SKB_GSO_UDP.

I realise that this particular patch is a long-standing bug that we
can fix (and will need backporting in our tree), but it's nice if we
can keep the patches applied to master here in the same order as
upstream net-next so that it's easier to tell how out of sync we are
by a side-by-side comparison of "git log --oneline" for datapath/ or
net/openvswitch in the corresponding trees. I also realise that this
patch didn't land in net-next yet since it's so new, but it's fairly
safe to assume it'll be applied there next.

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

Re: [ovs-dev] [PATCH 1/2] tests/system-offloads-traffic.at: add sanity check

[Joe Stringer]

On 17 August 2017 at 11:07, Joe Stringer <j...@ovn.org> wrote:
> On 17 August 2017 at 00:17, Roi Dayan <r...@mellanox.com> wrote:
>>
>>
>> On 17/08/2017 08:32, Roi Dayan wrote:
>>>
>>>
>>>
>>> On 17/08/2017 01:17, Joe Stringer wrote:
>>>>
>>>> On 16 August 2017 at 05:14, Roi Dayan <r...@mellanox.com> wrote:
>>>>>
>>>>> Doing dump-flows also altering the netdev ports list.
>>>>> So doing it pre the actual test is adding a check to
>>>>> make sure we don't break the that list.
>>>>>
>>>>> Signed-off-by: Roi Dayan <r...@mellanox.com>
>>>>> Reviewed-by: Paul Blakey <pa...@mellanox.com>
>>>>
>>>>
>>>> I'm actually not sure what the requirements are to run these offload
>>>> tests. I tried running them on a 4.4 kernel, and the first test passed
>>>> while the second failed; I assume that this is because 4.4's TC flower
>>>> support is not new enough.
>>>>
>>>> Then I tried with a 4.12 kernel and neither test passed, and I have
>>>> extra flows being reported in the dump-flows output.
>>>>
>>>> I believe that I understand what this patch is trying to achieve, but
>>>> I don't know how I'm supposed to validate it.
>>>>
>>>
>>
>> In the past you had an issue that cls_flower was not configured
>> in your kernel. could be the same issue now?
>
> Maybe it is, I changed which setup I was testing in. I'll double-check.

Managed to figure it out, looks good. Applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH V3 1/2] dpif: Fix cleanup of netdev_ports map

[Joe Stringer]

On 16 August 2017 at 22:59, Roi Dayan  wrote:
> Executing dpctl commands from userspace also calls to
> dpif_open()/dpif_close() but not really creating another dpif
> but using a clone.
> As for netdev_ports map is global we avoid adding duplicate entries
> but also need to make sure we are not removing needed entries.
> With this commit we make sure only the last dpif close should clean
> the netdev_ports map.
>
> Fixes: 6595cb95a4a9 ("dpif: Clean up netdev_ports map on dpif_close().")
> Signed-off-by: Roi Dayan 
> Reviewed-by: Paul Blakey 
> ---

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

Re: [ovs-dev] [PATCH 4/4] netdev-tc-offloads: Add support for action set

[Joe Stringer]

On 17 August 2017 at 00:52, Paul Blakey <pa...@mellanox.com> wrote:
>
>
> On 15/08/2017 20:24, Joe Stringer wrote:
>>
>> On 15 August 2017 at 01:19, Paul Blakey <pa...@mellanox.com> wrote:
>>>
>>>
>>>
>>> On 15/08/2017 10:28, Paul Blakey wrote:
>>>>
>>>>
>>>>
>>>>
>>>> On 15/08/2017 04:04, Joe Stringer wrote:
>>>>>
>>>>>
>>>>> On 8 August 2017 at 07:21, Roi Dayan <r...@mellanox.com> wrote:
>>>>>>
>>>>>>
>>>>>> From: Paul Blakey <pa...@mellanox.com>
>>>>>>
>>>>>> Implement support for offloading ovs action set using
>>>>>> tc header rewrite action.
>>>>>>
>>>>>> Signed-off-by: Paul Blakey <pa...@mellanox.com>
>>>>>> Reviewed-by: Roi Dayan <r...@mellanox.com>
>>>>>> ---
>>>>>
>>>>>
>>>>>
>>>>> 
>>>>>
>>>>>> @@ -274,6 +346,48 @@ netdev_tc_flow_dump_destroy(struct
>>>>>> netdev_flow_dump
>>>>>> *dump)
>>>>>>return 0;
>>>>>>}
>>>>>>
>>>>>> +static void
>>>>>> +parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf,
>>>>>> +   struct tc_flower *flower)
>>>>>> +{
>>>>>> +char *mask = (char *) >rewrite.mask;
>>>>>> +char *data = (char *) >rewrite.key;
>>>>>> +
>>>>>> +for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) {
>>>>>> +char *put = NULL;
>>>>>> +size_t nested = 0;
>>>>>> +int len = ovs_flow_key_attr_lens[type].len;
>>>>>> +
>>>>>> +if (len <= 0) {
>>>>>> +continue;
>>>>>> +}
>>>>>> +
>>>>>> +for (int j = 0; j < ARRAY_SIZE(set_flower_map[type]); j++) {
>>>>>> +struct netlink_field *f = _flower_map[type][j];
>>>>>> +
>>>>>> +if (!f->size) {
>>>>>> +break;
>>>>>> +}
>>>>>
>>>>>
>>>>>
>>>>> Seems like maybe there's similar feedback here to the previous patch
>>>>> wrt sparsely populated array set_flower_map[].
>>>>>
>>>>
>>>> I'll answer there.
>>>
>>>
>>>
>>> Scratch that, I actually meant to write it here.
>>>
>>> With this map we have fast (direct lookup) access on the put path
>>> (parse_put_flow_set_masked_action) , and slow access on the reverse dump
>>> path (parse_flower_rewrite_to_netlink_action) which iterates over all
>>> indices, although it should skips them  fast if they are empty.
>>>
>>> Changing this to sparse map will slow both of the paths, but dumping
>>> would
>>> be faster. We didn't write this maps for performance but for convince of
>>> adding new supported attributes.
>>
>>
>> OK, thanks for the explanation. I figured there must be a reason it
>> was indexed that way, I just didn't see it in my first review pass
>> through the series. Obviously fast path should be fast, and typically
>> we don't worry quite as much about dumping fast (although I have in
>> the past tested this to determine how many flows we will attempt to
>> populate in adverse conditions).
>>
>>> What we can do to both maps is:
>>>
>>> 1) Using ovs thread once, we can create a fast (direct lookup) access
>>> map;
>>> for both paths - generating a reverse map at init time and using that
>>> instead for dumping.
>>
>>
>> Until there's a clear, known advantage I'm not sure if we should do this.
>>
>>> 2) Rewrite it in a non sparse way, use it for both.
>>
>>
>> I think that we generally are more interested in 'put' performance so
>> if this sacrifices it for 'dump' performance, then it's probably not a
>> good idea (though it may still be too early to really optimize these).
>>
>>> 3) Rewrite it using a switch case, the logic for now is pretty much the
>>> same
>>> for all attributes.
>>>
>>> What do you think?
>>
>

Re: [ovs-dev] [dpdk-dev] [ovs-dpdk-tests] where is ovs-dpdk test case?

[Joe Stringer]

On 17 August 2017 at 05:07, Kavanagh, Mark B  wrote:
>
>>From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Sam
>>Sent: Thursday, August 17, 2017 7:31 AM
>>To: ovs-disc...@openvswitch.org; ovs-dev@openvswitch.org; d...@dpdk.org
>
> Hi Sam,
>
> Just a heads-up that d...@dpdk.org is strictly for DPDK development threads - 
> I've removed it from this thread.
>
> Also, responses to your queries are inline.
>
> Thanks!
> Mark
>
>
>>Subject: [dpdk-dev] [ovs-dpdk-tests] where is ovs-dpdk test case?
>>
>>Hi all,
>>
>>I'm working with ovs-dpdk, I want to run ovs-dpdk test case. But I found
>>there is no test case for 'netdev' type bridge and no test case for
>>ovs-dpdk datapath(which is pmd_thread_main).
>
> Do you mean unit tests (as in OvS autotests), or integration tests?
>
> If the former, then unfortunately there are none at present within OvS (but 
> feel free to add some!); if you'd like to run integration tests, you could 
> take a look at VSPERF: https://etherpad.opnfv.org/p/vSwitchIntegrationTests.

"make check" runs unit tests against the userspace datapath, and it
should work with a DPDK-enabled build of OVS. I don't know how much it
tests the DPDK-specific portions of this, however. For example, it
won't use DPDK devices.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCHv2] checkpatch: Enforce bracing around conditionals.

[Joe Stringer]

The coding style states that BSD-style brace placement should be used,
and even single statements should be enclosed. Add checks to checkpatch
for this, particularly for 'else' statements.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
v2: Combine in same check as if_and_for_end_with_bracket_check
---
 utilities/checkpatch.py | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 43f10bb3ded3..185ddaf0d5e9 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -95,6 +95,8 @@ __regex_ends_with_bracket = \
 __regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*[^*]')
 __regex_is_comment_line = re.compile(r'^\s*(/\*|\*\s)')
 __regex_trailing_operator = re.compile(r'^[^ ]* [^ ]*[?:]$')
+__regex_conditional_else_bracing = re.compile(r'^\s*else\s*{?$')
+__regex_conditional_else_bracing2 = re.compile(r'^\s*}\selse\s*$')
 
 skip_leading_whitespace_check = False
 skip_trailing_whitespace_check = False
@@ -186,6 +188,10 @@ def if_and_for_end_with_bracket_check(line):
 return True
 if __regex_ends_with_bracket.search(line) is None:
 return False
+if __regex_conditional_else_bracing.match(line) is not None:
+return False
+if __regex_conditional_else_bracing2.match(line) is not None:
+return False
 return True
 
 
-- 
2.14.1

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

Re: [ovs-dev] [PATCH] checkpatch: Enforce bracing around conditionals.

[Joe Stringer]

On 17 August 2017 at 07:35, Aaron Conole <acon...@redhat.com> wrote:
> Hi Joe,
>
> Joe Stringer <j...@ovn.org> writes:
>
>> The coding style states that BSD-style brace placement should be used,
>> and even single statements should be enclosed. Add checks to checkpatch
>> for this.
>>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>>  utilities/checkpatch.py | 13 +
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 43f10bb3ded3..c8381c98403b 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -95,6 +95,8 @@ __regex_ends_with_bracket = \
>>  __regex_ptr_declaration_missing_whitespace = 
>> re.compile(r'[a-zA-Z0-9]\*[^*]')
>>  __regex_is_comment_line = re.compile(r'^\s*(/\*|\*\s)')
>>  __regex_trailing_operator = re.compile(r'^[^ ]* [^ ]*[?:]$')
>> +__regex_conditional_else_bracing = re.compile(r'^\s*else\s*{?$')
>> +__regex_conditional_else_bracing2 = re.compile(r'^\s*}\selse\s*$')
>>
>>  skip_leading_whitespace_check = False
>>  skip_trailing_whitespace_check = False
>> @@ -212,6 +214,12 @@ def trailing_operator(line):
>>  return __regex_trailing_operator.match(line) is not None
>>
>>
>> +def if_else_bracing_check(line):
>> +if __regex_conditional_else_bracing.match(line) is not None:
>> +return True
>> +return __regex_conditional_else_bracing2.match(line) is not None
>> +
>> +
>
> Would it make more sense to put this in with the other bracing checks in
> if_and_for_end_with_bracket_check ?

Yes! Apparently I missed this before. Thanks, I'll respin.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] tests/system-offloads-traffic.at: add sanity check

[Joe Stringer]

On 17 August 2017 at 00:17, Roi Dayan <r...@mellanox.com> wrote:
>
>
> On 17/08/2017 08:32, Roi Dayan wrote:
>>
>>
>>
>> On 17/08/2017 01:17, Joe Stringer wrote:
>>>
>>> On 16 August 2017 at 05:14, Roi Dayan <r...@mellanox.com> wrote:
>>>>
>>>> Doing dump-flows also altering the netdev ports list.
>>>> So doing it pre the actual test is adding a check to
>>>> make sure we don't break the that list.
>>>>
>>>> Signed-off-by: Roi Dayan <r...@mellanox.com>
>>>> Reviewed-by: Paul Blakey <pa...@mellanox.com>
>>>
>>>
>>> I'm actually not sure what the requirements are to run these offload
>>> tests. I tried running them on a 4.4 kernel, and the first test passed
>>> while the second failed; I assume that this is because 4.4's TC flower
>>> support is not new enough.
>>>
>>> Then I tried with a 4.12 kernel and neither test passed, and I have
>>> extra flows being reported in the dump-flows output.
>>>
>>> I believe that I understand what this patch is trying to achieve, but
>>> I don't know how I'm supposed to validate it.
>>>
>>
>
> In the past you had an issue that cls_flower was not configured
> in your kernel. could be the same issue now?

Maybe it is, I changed which setup I was testing in. I'll double-check.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] checkpatch: Enforce bracing around conditionals.

[Joe Stringer]

The coding style states that BSD-style brace placement should be used,
and even single statements should be enclosed. Add checks to checkpatch
for this.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
 utilities/checkpatch.py | 13 +
 1 file changed, 13 insertions(+)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 43f10bb3ded3..c8381c98403b 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -95,6 +95,8 @@ __regex_ends_with_bracket = \
 __regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*[^*]')
 __regex_is_comment_line = re.compile(r'^\s*(/\*|\*\s)')
 __regex_trailing_operator = re.compile(r'^[^ ]* [^ ]*[?:]$')
+__regex_conditional_else_bracing = re.compile(r'^\s*else\s*{?$')
+__regex_conditional_else_bracing2 = re.compile(r'^\s*}\selse\s*$')
 
 skip_leading_whitespace_check = False
 skip_trailing_whitespace_check = False
@@ -212,6 +214,12 @@ def trailing_operator(line):
 return __regex_trailing_operator.match(line) is not None
 
 
+def if_else_bracing_check(line):
+if __regex_conditional_else_bracing.match(line) is not None:
+return True
+return __regex_conditional_else_bracing2.match(line) is not None
+
+
 checks = [
 {'regex': None,
  'match_name':
@@ -250,6 +258,11 @@ checks = [
  'check': lambda x: trailing_operator(x),
  'print':
  lambda: print_error("Line has '?' or ':' operator at end of line")},
+
+{'regex': '(.c|.h)(.in)?$', 'match_name': None,
+ 'prereq': lambda x: not is_comment_line(x),
+ 'check': lambda x: if_else_bracing_check(x),
+ 'print': lambda: print_error("Improper bracing in conditional statement")}
 ]
 
 
-- 
2.14.1

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

Re: [ovs-dev] [PATCH] netdev-tc-offloads: Add prefix to identify source of log msg

[Joe Stringer]

On 16 August 2017 at 05:27, Roi Dayan  wrote:
> There is an identical log msg from multiple api calls.
> Add a prefix to identify the source function of the log msg.
>
> Signed-off-by: Roi Dayan 
> Reviewed-by: Paul Blakey 
> ---

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

Re: [ovs-dev] [PATCH 1/2] tests/system-offloads-traffic.at: add sanity check

[Joe Stringer]

On 16 August 2017 at 05:14, Roi Dayan  wrote:
> Doing dump-flows also altering the netdev ports list.
> So doing it pre the actual test is adding a check to
> make sure we don't break the that list.
>
> Signed-off-by: Roi Dayan 
> Reviewed-by: Paul Blakey 

I'm actually not sure what the requirements are to run these offload
tests. I tried running them on a 4.4 kernel, and the first test passed
while the second failed; I assume that this is because 4.4's TC flower
support is not new enough.

Then I tried with a 4.12 kernel and neither test passed, and I have
extra flows being reported in the dump-flows output.

I believe that I understand what this patch is trying to achieve, but
I don't know how I'm supposed to validate it.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH V3 2/2] dpif: Fix cleanup of netdev_ports map

[Joe Stringer]

On 16 August 2017 at 05:12, Roi Dayan  wrote:
> Executing dpctl commands from userspace also calls to
> dpif_open()/dpif_close() but not really creating another dpif
> but using a clone.
> As for netdev_ports map is global we avoid adding duplicate entries
> but also need to make sure we are not removing needed entries.
> With this commit we make sure only the last dpif close should clean
> the netdev_ports map.
>
> Fixes: 6595cb95a4a9 ("dpif: Clean up netdev_ports map on dpif_close().")
> Signed-off-by: Roi Dayan 
> Reviewed-by: Paul Blakey 
> ---

Thanks Roi.

Usually we apply the test that shows the failure after we apply the
fix, so that the breakage isn't introduced anywhere on the tree - so
the patches would be rearranged.

Can we also roll in the following style incremental?

diff --git a/lib/dpif.c b/lib/dpif.c
index 121a26db0c37..0c8b91b68b24 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -428,8 +428,8 @@ dpif_create_and_open(const char *name, const char
*type, struct dpif **dpifp)
return error;
}

-static
-void dpif_remove_netdev_ports(struct dpif *dpif) {
+static void
+dpif_remove_netdev_ports(struct dpif *dpif) {
struct dpif_port_dump port_dump;
struct dpif_port dpif_port;
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 2/2] tests: Comment which netcat version the opts are for.

[Joe Stringer]

On 15 August 2017 at 16:45, Flavio Leitner <f...@sysclose.org> wrote:
> On Tue, 15 Aug 2017 16:15:55 -0700
> Joe Stringer <j...@ovn.org> wrote:
>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>>  tests/atlocal.in | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tests/atlocal.in b/tests/atlocal.in
>> index ba143d30f1d7..7c5e9e3357e5 100644
>> --- a/tests/atlocal.in
>> +++ b/tests/atlocal.in
>> @@ -152,8 +152,10 @@ find_command nc
>>
>>  # Determine correct netcat option to quit on stdin EOF
>>  if nc --version 2>&1 | grep -q nmap.org; then
>> +# Nmap netcat
>>  NC_EOF_OPT="--send-only -w 5"
>>  else
>> +# BSD netcat
>>  NC_EOF_OPT="-q 1 -w 5"
>>  fi
>>
>
> Acked-by: Flavio Leitner <f...@sysclose.org>

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 1/2] tests: Put maximum timeout on netcat calls.

[Joe Stringer]

On 15 August 2017 at 16:45, Flavio Leitner <f...@sysclose.org> wrote:
> On Tue, 15 Aug 2017 16:15:54 -0700
> Joe Stringer <j...@ovn.org> wrote:
>
>> This was causing test script execution to hang forever on Ubuntu Zesty.
>> Make sure it times out within 5 seconds, so at least it will fail out
>> properly.
>>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>>  tests/atlocal.in | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/atlocal.in b/tests/atlocal.in
>> index 6a339f8fc312..ba143d30f1d7 100644
>> --- a/tests/atlocal.in
>> +++ b/tests/atlocal.in
>> @@ -152,9 +152,9 @@ find_command nc
>>
>>  # Determine correct netcat option to quit on stdin EOF
>>  if nc --version 2>&1 | grep -q nmap.org; then
>> -NC_EOF_OPT="--send-only"
>> +NC_EOF_OPT="--send-only -w 5"
>>  else
>> -NC_EOF_OPT="-q 1"
>> +NC_EOF_OPT="-q 1 -w 5"
>>  fi
>>
>>  # Set HAVE_TCPDUMP
>
>
> make check-system-userspace TESTSUITEFLAGS="35"
> ## -- ##
> ## openvswitch 2.8.90 test suite. ##
> ## -- ##
>  35: conntrack - ICMP relatedok
>
> ## - ##
> ## Test results. ##
> ## - ##
>
> 1 test was successful.
>
> in parallel:
> ps auwx | grep nc | grep 'send-'
> root  2379  0.0  0.0   9680  2508 pts/25   S+   20:42   0:00 bash -c echo 
> a | nc --send-only -w 5 -u 10.1.1.2 1
>
> Acked-by: Flavio Leitner <f...@sysclose.org>

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCHv2 2/2] tests: Comment which netcat version the opts are for.

[Joe Stringer]

Signed-off-by: Joe Stringer <j...@ovn.org>
---
 tests/atlocal.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/atlocal.in b/tests/atlocal.in
index ba143d30f1d7..7c5e9e3357e5 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -152,8 +152,10 @@ find_command nc
 
 # Determine correct netcat option to quit on stdin EOF
 if nc --version 2>&1 | grep -q nmap.org; then
+# Nmap netcat
 NC_EOF_OPT="--send-only -w 5"
 else
+# BSD netcat
 NC_EOF_OPT="-q 1 -w 5"
 fi
 
-- 
2.13.3

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

Re: [ovs-dev] [PATCH] tests: Put maximum timeout on netcat calls.

[Joe Stringer]

On 15 August 2017 at 15:23, Flavio Leitner <f...@sysclose.org> wrote:
> On Tue, 15 Aug 2017 15:03:25 -0700
> Joe Stringer <j...@ovn.org> wrote:
>
>> This was causing test script execution to hang forever on Ubuntu Zesty.
>> Make sure it times out within 5 seconds, so at least it will fail out
>> properly.
>>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>>  tests/atlocal.in | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/atlocal.in b/tests/atlocal.in
>> index 6a339f8fc312..6ac32fbb3d73 100644
>> --- a/tests/atlocal.in
>> +++ b/tests/atlocal.in
>> @@ -154,7 +154,8 @@ find_command nc
>>  if nc --version 2>&1 | grep -q nmap.org; then
>>  NC_EOF_OPT="--send-only"
>>  else
>> -NC_EOF_OPT="-q 1"
>> +# BSD netcat
>> +NC_EOF_OPT="-q 1 -w 5"
>>  fi
>>
>>  # Set HAVE_TCPDUMP
>
> Why not fixing on both?
>
> $ nc --version
> Ncat: Version 7.40 ( https://nmap.org/ncat )
>
> $ nc --help  | grep -- -w
>   -w, --wait   Connect timeout

Mostly because I didn't have the nmap version on hand so didn't know
if it worked ;)

Thanks, I'll update this.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] tests: Put maximum timeout on netcat calls.

[Joe Stringer]

This was causing test script execution to hang forever on Ubuntu Zesty.
Make sure it times out within 5 seconds, so at least it will fail out
properly.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
 tests/atlocal.in | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/atlocal.in b/tests/atlocal.in
index 6a339f8fc312..6ac32fbb3d73 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -154,7 +154,8 @@ find_command nc
 if nc --version 2>&1 | grep -q nmap.org; then
 NC_EOF_OPT="--send-only"
 else
-NC_EOF_OPT="-q 1"
+# BSD netcat
+NC_EOF_OPT="-q 1 -w 5"
 fi
 
 # Set HAVE_TCPDUMP
-- 
2.13.3

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

Re: [ovs-dev] [PATCHv3 0/4] Improve C++ support for OVSDB IDL.

[Joe Stringer]

On 11 August 2017 at 11:06, Joe Stringer <j...@ovn.org> wrote:
> In the OVSDB IDL, we use C++ keywords such as "new", "mutable", "class"
> for variable and field names. This series updates these names so that
> they don't conflict with the C++ keywords, which should improve the
> ability to use the IDL from C++ code. This series focuses primarily on
> code that exists in the tree; To address such problems for generated
> field names that come from an OVSDB schema, a subsequent patch will
> still be required. For instance, the ovs-vswitchd schema has a column
> named "protected". This ends up as a field name in the generated
> lib/vswitch-idl.[ch] code, which causes similar problems to those
> addressed by this series.

Thanks folks, I pushed this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/4] netdev-tc-offloads: Add support for action set

[Joe Stringer]

On 8 August 2017 at 07:21, Roi Dayan  wrote:
> From: Paul Blakey 
>
> Implement support for offloading ovs action set using
> tc header rewrite action.
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> ---



Another couple of bits I noticed while responding..

> @@ -454,10 +572,56 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>  }
>
>  static int
> +parse_put_flow_set_masked_action(struct tc_flower *flower,
> + const struct nlattr *set,
> + size_t set_len,
> + bool hasmask)
> +{
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +const struct nlattr *set_attr;
> +char *key = (char *) >rewrite.key;
> +char *mask = (char *) >rewrite.mask;
> +size_t set_left;
> +int i, j;
> +
> +NL_ATTR_FOR_EACH_UNSAFE(set_attr, set_left, set, set_len) {
> +int type = nl_attr_type(set_attr);
> +size_t size = nl_attr_get_size(set_attr) / 2;
> +char *set_data = CONST_CAST(char *, nl_attr_get(set_attr));
> +char *set_mask = set_data + size;
> +
> +if (type >= ARRAY_SIZE(set_flower_map)) {
> +VLOG_DBG_RL(, "unsupported set action type: %d", type);
> +return EOPNOTSUPP;

I think that this assumes that 'set_flower_map' has every entry
populated up until the maximum supported key field - but are some
missing - for example OVS_KEY_ATTR_VLAN. Could we also check by
indexing into set_flower_map and checking if it's a valid entry?

> +}
> +
> +for (i = 0; i < ARRAY_SIZE(set_flower_map[type]); i++) {
> +struct netlink_field *f = _flower_map[type][i];

Separate thought - you're iterating nlattrs above then iterating all
the key attributes in the flower_map here. Couldn't you just directly
index into the packet field that this action will modify?

> +
> +if (!f->size) {
> +break;
> +}

I think that headers that are not in the set_flower_map will hit this,
which would be unsupported (similar to above comment).

> +
> +for (j = 0; j < f->size; j++) {
> +char maskval = hasmask ? set_mask[f->offset + j] : 0xFF;
> +
> +key[f->flower_offset + j] = maskval & set_data[f->offset + 
> j];
> +mask[f->flower_offset + j] = maskval;
> +}
> +}
> +}
> +
> +if (!is_all_zeros(>rewrite, sizeof flower->rewrite)) {
> +flower->rewrite.rewrite = true;
> +}
> +
> +return 0;
> +}
> +
> +static int
>  parse_put_flow_set_action(struct tc_flower *flower, const struct nlattr *set,
>size_t set_len)
>  {
> -static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>  const struct nlattr *set_attr;
>  size_t set_left;
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/4] netdev-tc-offloads: Add support for action set

[Joe Stringer]

On 15 August 2017 at 01:19, Paul Blakey <pa...@mellanox.com> wrote:
>
>
> On 15/08/2017 10:28, Paul Blakey wrote:
>>
>>
>>
>> On 15/08/2017 04:04, Joe Stringer wrote:
>>>
>>> On 8 August 2017 at 07:21, Roi Dayan <r...@mellanox.com> wrote:
>>>>
>>>> From: Paul Blakey <pa...@mellanox.com>
>>>>
>>>> Implement support for offloading ovs action set using
>>>> tc header rewrite action.
>>>>
>>>> Signed-off-by: Paul Blakey <pa...@mellanox.com>
>>>> Reviewed-by: Roi Dayan <r...@mellanox.com>
>>>> ---
>>>
>>>
>>> 
>>>
>>>> @@ -274,6 +346,48 @@ netdev_tc_flow_dump_destroy(struct netdev_flow_dump
>>>> *dump)
>>>>   return 0;
>>>>   }
>>>>
>>>> +static void
>>>> +parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf,
>>>> +   struct tc_flower *flower)
>>>> +{
>>>> +char *mask = (char *) >rewrite.mask;
>>>> +char *data = (char *) >rewrite.key;
>>>> +
>>>> +for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) {
>>>> +char *put = NULL;
>>>> +size_t nested = 0;
>>>> +int len = ovs_flow_key_attr_lens[type].len;
>>>> +
>>>> +if (len <= 0) {
>>>> +continue;
>>>> +}
>>>> +
>>>> +for (int j = 0; j < ARRAY_SIZE(set_flower_map[type]); j++) {
>>>> +struct netlink_field *f = _flower_map[type][j];
>>>> +
>>>> +if (!f->size) {
>>>> +break;
>>>> +}
>>>
>>>
>>> Seems like maybe there's similar feedback here to the previous patch
>>> wrt sparsely populated array set_flower_map[].
>>>
>>
>> I'll answer there.
>
>
> Scratch that, I actually meant to write it here.
>
> With this map we have fast (direct lookup) access on the put path
> (parse_put_flow_set_masked_action) , and slow access on the reverse dump
> path (parse_flower_rewrite_to_netlink_action) which iterates over all
> indices, although it should skips them  fast if they are empty.
>
> Changing this to sparse map will slow both of the paths, but dumping would
> be faster. We didn't write this maps for performance but for convince of
> adding new supported attributes.

OK, thanks for the explanation. I figured there must be a reason it
was indexed that way, I just didn't see it in my first review pass
through the series. Obviously fast path should be fast, and typically
we don't worry quite as much about dumping fast (although I have in
the past tested this to determine how many flows we will attempt to
populate in adverse conditions).

> What we can do to both maps is:
>
> 1) Using ovs thread once, we can create a fast (direct lookup) access map
> for both paths - generating a reverse map at init time and using that
> instead for dumping.

Until there's a clear, known advantage I'm not sure if we should do this.

> 2) Rewrite it in a non sparse way, use it for both.

I think that we generally are more interested in 'put' performance so
if this sacrifices it for 'dump' performance, then it's probably not a
good idea (though it may still be too early to really optimize these).

> 3) Rewrite it using a switch case, the logic for now is pretty much the same
> for all attributes.
>
> What do you think?

Do you think that this third option would improve the readability,
allow more compile-time checking on the code, or more code reuse?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/4] tc: Add header rewrite using tc pedit action

[Joe Stringer]

On 15 August 2017 at 01:19, Paul Blakey <pa...@mellanox.com> wrote:
>
>
> On 15/08/2017 00:56, Joe Stringer wrote:
>>
>> On 8 August 2017 at 07:21, Roi Dayan <r...@mellanox.com> wrote:
>>>
>>> From: Paul Blakey <pa...@mellanox.com>
>>>
>>> @@ -346,6 +425,96 @@ nl_parse_flower_ip(struct nlattr **attrs, struct
>>> tc_flower *flower) {
>>>   }
>>>   }
>>>
>>> +static const struct nl_policy pedit_policy[] = {
>>> +[TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC,
>>> + .min_len = sizeof(struct tc_pedit),
>>> + .optional = false, },
>>> +[TCA_PEDIT_KEYS_EX]   = { .type = NL_A_NESTED,
>>> +  .optional = false, },
>>> +};
>>> +
>>> +static int
>>> +nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower)
>>> +{
>>> +struct nlattr *pe_attrs[ARRAY_SIZE(pedit_policy)];
>>> +const struct tc_pedit *pe;
>>> +const struct tc_pedit_key *keys;
>>> +const struct nlattr *nla, *keys_ex, *ex_type;
>>> +const void *keys_attr;
>>> +char *rewrite_key = (void *) >rewrite.key;
>>> +char *rewrite_mask = (void *) >rewrite.mask;
>>
>>
>> These are actually 'struct tc_flower_key', shouldn't we use the actual
>> types? (I realise there is pointer arithmetic below, but usually we
>> restrict the usage of void casting and pointer arithmetic as much as
>> possible).
>>
>
> Yes, It's for the pointer arithmetic. (void *) cast was for the
> clangs warning "cast increases required alignment of target type"
> which we couldn't find another way to suppress.
> I don't think alignments an issue here.

Ah. I don't have particularly much experience with pointer alignment.

>
>>> +size_t keys_ex_size, left;
>>> +int type, i = 0;
>>> +
>>> +if (!nl_parse_nested(options, pedit_policy, pe_attrs,
>>> + ARRAY_SIZE(pedit_policy))) {
>>> +VLOG_ERR_RL(_rl, "failed to parse pedit action options");
>>> +return EPROTO;
>>> +}
>>> +
>>> +pe = nl_attr_get_unspec(pe_attrs[TCA_PEDIT_PARMS_EX], sizeof *pe);
>>> +keys = pe->keys;
>>> +keys_attr = pe_attrs[TCA_PEDIT_KEYS_EX];
>>> +keys_ex = nl_attr_get(keys_attr);
>>> +keys_ex_size = nl_attr_get_size(keys_attr);
>>> +
>>> +NL_ATTR_FOR_EACH(nla, left, keys_ex, keys_ex_size) {
>>> +if (i >= pe->nkeys) {
>>> +break;
>>> +}
>>> +
>>> +if (nl_attr_type(nla) == TCA_PEDIT_KEY_EX) {
>>> +ex_type = nl_attr_find_nested(nla, TCA_PEDIT_KEY_EX_HTYPE);
>>> +type = nl_attr_get_u16(ex_type);
>>> +
>>> +for (int j = 0; j < ARRAY_SIZE(flower_pedit_map); j++) {
>>> +struct flower_key_to_pedit *m = _pedit_map[j];
>>> +int flower_off = j;
>>> +int sz = m->size;
>>> +int mf = m->offset;
>>> +
>>> +if (!sz || m->htype != type) {
>>> +   continue;
>>> +}
>>
>>
>> If flower_pedit_map was just a regular array and the offset was
>> included in 'struct flower_key_to_pedit', then I don't think we need
>> the above check?
>>
>>> +
>>> +/* check overlap between current pedit key, which is
>>> always
>>> + * 4 bytes (range [off, off + 3]), and a map entry in
>>> + * flower_pedit_map (range [mf, mf + sz - 1]) */
>>> +if ((keys->off >= mf && keys->off < mf + sz)
>>> +|| (keys->off + 3 >= mf && keys->off + 3 < mf + sz))
>>> {
>>> +int diff = flower_off + (keys->off - mf);
>>> +uint32_t *dst = (void *) (rewrite_key + diff);
>>> +uint32_t *dst_m = (void *) (rewrite_mask + diff);
>>> +uint32_t mask = ~(keys->mask);
>>> +uint32_t zero_bits;
>>> +
>>> +if (keys->off < mf) {
>>> +zero_bits = 8 * (mf - keys->off);
>>> +mask &= UINT32_MAX << zero_bits;
>>>

Re: [ovs-dev] [PATCH 4/4] netdev-tc-offloads: Add support for action set

[Joe Stringer]

On 8 August 2017 at 07:21, Roi Dayan  wrote:
> From: Paul Blakey 
>
> Implement support for offloading ovs action set using
> tc header rewrite action.
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> ---



> @@ -274,6 +346,48 @@ netdev_tc_flow_dump_destroy(struct netdev_flow_dump 
> *dump)
>  return 0;
>  }
>
> +static void
> +parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf,
> +   struct tc_flower *flower)
> +{
> +char *mask = (char *) >rewrite.mask;
> +char *data = (char *) >rewrite.key;
> +
> +for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) {
> +char *put = NULL;
> +size_t nested = 0;
> +int len = ovs_flow_key_attr_lens[type].len;
> +
> +if (len <= 0) {
> +continue;
> +}
> +
> +for (int j = 0; j < ARRAY_SIZE(set_flower_map[type]); j++) {
> +struct netlink_field *f = _flower_map[type][j];
> +
> +if (!f->size) {
> +break;
> +}

Seems like maybe there's similar feedback here to the previous patch
wrt sparsely populated array set_flower_map[].

> +
> +if (!is_all_zeros(mask + f->flower_offset, f->size)) {
> +if (!put) {
> +nested = nl_msg_start_nested(buf,
> + OVS_ACTION_ATTR_SET_MASKED);
> +put = nl_msg_put_unspec_zero(buf, type, len * 2);

Do we ever have multiple of these attributes in a single nested
OVS_ACTION_ATTR_SET_MASKED? If so, won't the following memcpys avoid
putting the netlink header for the subsequent sets?

> +}
> +
> +memcpy(put + f->offset, data + f->flower_offset, f->size);
> +memcpy(put + len + f->offset,
> +   mask + f->flower_offset, f->size);

Could we these combine with the nl_msg_put_unspec() above?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/4] tc: Add header rewrite using tc pedit action

[Joe Stringer]

On 8 August 2017 at 07:21, Roi Dayan  wrote:
> From: Paul Blakey 
>
> To be later used to implement ovs action set offloading.
>
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> ---

Hi Paul and folks, some questions inline.

utilities/checkpatch.py says:

$ utilities/checkpatch.py -4
== Checking 00ecb482f5d1 ("compat: Add act_pedit compatibility for old
kernels") ==
Lines checked: 127, no obvious problems found

== Checking 7503746718f6 ("odp-util: Expose ovs flow key attr len
table for reuse") ==
Lines checked: 195, no obvious problems found

== Checking 1e8ab68aefe1 ("tc: Add header rewrite using tc pedit action") ==
ERROR: Improper whitespace around control block
#359 FILE: lib/tc.c:480:
   NL_ATTR_FOR_EACH(nla, left, keys_ex, keys_ex_size) {

Lines checked: 721, Warnings: 0, Errors: 1

== Checking f6b52ce504c2 ("netdev-tc-offloads: Add support for action set") ==
ERROR: Improper whitespace around control block
#908 FILE: lib/netdev-tc-offloads.c:590:
   NL_ATTR_FOR_EACH_UNSAFE(set_attr, set_left, set, set_len) {

Lines checked: 981, Warnings: 0, Errors: 1



> +static struct flower_key_to_pedit flower_pedit_map[] = {
> +[offsetof(struct tc_flower_key, ipv4.ipv4_src)] = {
> +TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> +12,
> +MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_src)
> +},

Why are these items indexed by the offset, and not just an array we
iterate? It seems like the only places where we iterate this, we use
"for (i=0; ... i++)", which means that we iterate several times across
empty items, and this array is sparsely populated.

> +[offsetof(struct tc_flower_key, ipv4.ipv4_dst)] = {
> +TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> +16,
> +MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_dst)
> +},
> +[offsetof(struct tc_flower_key, ipv4.rewrite_ttl)] = {
> +TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> +8,
> +MEMBER_SIZEOF(struct tc_flower_key, ipv4.rewrite_ttl)
> +},



> @@ -346,6 +425,96 @@ nl_parse_flower_ip(struct nlattr **attrs, struct 
> tc_flower *flower) {
>  }
>  }
>
> +static const struct nl_policy pedit_policy[] = {
> +[TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC,
> + .min_len = sizeof(struct tc_pedit),
> + .optional = false, },
> +[TCA_PEDIT_KEYS_EX]   = { .type = NL_A_NESTED,
> +  .optional = false, },
> +};
> +
> +static int
> +nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower)
> +{
> +struct nlattr *pe_attrs[ARRAY_SIZE(pedit_policy)];
> +const struct tc_pedit *pe;
> +const struct tc_pedit_key *keys;
> +const struct nlattr *nla, *keys_ex, *ex_type;
> +const void *keys_attr;
> +char *rewrite_key = (void *) >rewrite.key;
> +char *rewrite_mask = (void *) >rewrite.mask;

These are actually 'struct tc_flower_key', shouldn't we use the actual
types? (I realise there is pointer arithmetic below, but usually we
restrict the usage of void casting and pointer arithmetic as much as
possible).

> +size_t keys_ex_size, left;
> +int type, i = 0;
> +
> +if (!nl_parse_nested(options, pedit_policy, pe_attrs,
> + ARRAY_SIZE(pedit_policy))) {
> +VLOG_ERR_RL(_rl, "failed to parse pedit action options");
> +return EPROTO;
> +}
> +
> +pe = nl_attr_get_unspec(pe_attrs[TCA_PEDIT_PARMS_EX], sizeof *pe);
> +keys = pe->keys;
> +keys_attr = pe_attrs[TCA_PEDIT_KEYS_EX];
> +keys_ex = nl_attr_get(keys_attr);
> +keys_ex_size = nl_attr_get_size(keys_attr);
> +
> +NL_ATTR_FOR_EACH(nla, left, keys_ex, keys_ex_size) {
> +if (i >= pe->nkeys) {
> +break;
> +}
> +
> +if (nl_attr_type(nla) == TCA_PEDIT_KEY_EX) {
> +ex_type = nl_attr_find_nested(nla, TCA_PEDIT_KEY_EX_HTYPE);
> +type = nl_attr_get_u16(ex_type);
> +
> +for (int j = 0; j < ARRAY_SIZE(flower_pedit_map); j++) {
> +struct flower_key_to_pedit *m = _pedit_map[j];
> +int flower_off = j;
> +int sz = m->size;
> +int mf = m->offset;
> +
> +if (!sz || m->htype != type) {
> +   continue;
> +}

If flower_pedit_map was just a regular array and the offset was
included in 'struct flower_key_to_pedit', then I don't think we need
the above check?

> +
> +/* check overlap between current pedit key, which is always
> + * 4 bytes (range [off, off + 3]), and a map entry in
> + * flower_pedit_map (range [mf, mf + sz - 1]) */
> +if ((keys->off >= mf && keys->off < mf + sz)
> +|| (keys->off + 3 >= mf && keys->off + 3 < mf + sz)) {
> +int diff = flower_off + (keys->off - mf);
> +

Re: [ovs-dev] [PATCH 0/7] Add offload support for ip ttl and tcp flags

[Joe Stringer]

On 8 August 2017 at 01:03, Simon Horman  wrote:
> On Mon, Aug 07, 2017 at 06:19:04PM +0300, Roi Dayan wrote:
>> Hi,
>>
>> This series adds support for offloading ip ttl and tcp flags
>> using tc interface.
>
> This looks nice, thanks.
>
> Acked-by: Simon Horman 
>
> I'm also happy to apply this if someone else provides a review.

Thanks folks, I'll apply the series to master soon with Simon's acks
and the following style fixup.

diff --git a/lib/tc.c b/lib/tc.c
index bf12a5bea494..c9cada249de3 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -363,7 +363,6 @@ nl_parse_flower_ip(struct nlattr **attrs, struct
tc_flower *flower) {
 key->ip_ttl = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_IP_TTL]);
 mask->ip_ttl = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_IP_TTL_MASK]);
 }
-
 }

 static const struct nl_policy tunnel_key_policy[] = {
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCHv3 4/4] ovsdb-idl: Rename 'old' to 'old_datum'.

[Joe Stringer]

Now that the 'new' datum is named 'new_datum', be more consistent by
renaming 'old' to 'old_datum' to match.

Signed-off-by: Joe Stringer <j...@ovn.org>
Acked-by: Ben Pfaff <b...@ovn.org>
---
v3: Add ack.
v2: New patch.
---
 lib/ovsdb-data.h |  4 +--
 lib/ovsdb-idl-provider.h | 22 +++---
 lib/ovsdb-idl.c  | 74 +---
 3 files changed, 51 insertions(+), 49 deletions(-)

diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index 756219e9f497..72c8fe35bce3 100644
--- a/lib/ovsdb-data.h
+++ b/lib/ovsdb-data.h
@@ -221,12 +221,12 @@ void ovsdb_datum_subtract(struct ovsdb_datum *a,
 
 /* Generate and apply diffs */
 void ovsdb_datum_diff(struct ovsdb_datum *diff,
-  const struct ovsdb_datum *old,
+  const struct ovsdb_datum *old_datum,
   const struct ovsdb_datum *new_datum,
   const struct ovsdb_type *type);
 
 struct ovsdb_error *ovsdb_datum_apply_diff(struct ovsdb_datum *new_datum,
-   const struct ovsdb_datum *old,
+   const struct ovsdb_datum *old_datum,
const struct ovsdb_datum *diff,
const struct ovsdb_type *type)
 OVS_WARN_UNUSED_RESULT;
diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index ca6f58c9d763..a3eccb4c0ce6 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -34,19 +34,19 @@
  *
  * When no transaction is in progress:
  *
- * - 'old' points to the data committed to the database and currently
+ * - 'old_datum' points to the data committed to the database and currently
  *   in the row.
  *
- * - 'new_datum == old'.
+ * - 'new_datum == old_datum'.
  *
  * When a transaction is in progress, the situation is a little different.  For
- * a row inserted in the transaction, 'old' is NULL and 'new_datum' points to
- * the row's initial contents.  Otherwise:
+ * a row inserted in the transaction, 'old_datum' is NULL and 'new_datum'
+ * points to the row's initial contents.  Otherwise:
  *
- * - 'old' points to the data committed to the database and currently in
- *   the row.  (This is the same as when no transaction is in progress.)
+ * - 'old_datum' points to the data committed to the database and currently
+ *   in the row.  (This is the same as when no transaction is in progress.)
  *
- * - If the transaction does not modify the row, 'new_datum == old'.
+ * - If the transaction does not modify the row, 'new_datum == old_datum'.
  *
  * - If the transaction modifies the row, 'new_datum' points to the
  *   modified data.
@@ -55,8 +55,8 @@
  *
  * Thus:
  *
- * - 'old' always points to committed data, except that it is NULL if the
- *   row is inserted within the current transaction.
+ * - 'old_datum' always points to committed data, except that it is NULL if
+ *   the row is inserted within the current transaction.
  *
  * - 'new_datum' always points to the newest, possibly uncommitted version
  *   of the row's data, except that it is NULL if the row is deleted within
@@ -68,11 +68,11 @@ struct ovsdb_idl_row {
 struct ovs_list src_arcs;   /* Forward arcs (ovsdb_idl_arc.src_node). */
 struct ovs_list dst_arcs;   /* Backward arcs (ovsdb_idl_arc.dst_node). */
 struct ovsdb_idl_table *table; /* Containing table. */
-struct ovsdb_datum *old;/* Committed data (null if orphaned). */
+struct ovsdb_datum *old_datum; /* Committed data (null if orphaned). */
 
 /* Transactional data. */
 struct ovsdb_datum *new_datum; /* Modified data (null to delete row). */
-unsigned long int *prereqs; /* Bitmap of columns to verify in "old". */
+unsigned long int *prereqs; /* Bitmap of "old_datum" columns to verify. */
 unsigned long int *written; /* Bitmap of "new_datum" columns to write. */
 struct hmap_node txn_node;  /* Node in ovsdb_idl_txn's list. */
 unsigned long int *map_op_written; /* Bitmap of columns pending map ops. */
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index a1c83622ddb7..af1821bfff9c 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1907,7 +1907,7 @@ ovsdb_idl_row_change__(struct ovsdb_idl_row *row, const 
struct json *row_json,
 }
 
 column_idx = column - table->class_->columns;
-old = >old[column_idx];
+old = >old_datum[column_idx];
 
 error = NULL;
 if (apply_diff) {
@@ -1992,7 +1992,7 @@ ovsdb_idl_row_apply_diff(struct ovsdb_idl_row *row,
 static bool
 ovsdb_idl_row_is_orphan(const struct ovsdb_idl_row *row)
 {
-return !row->old && !row->new_datum;
+return !row->old_datum && !row->new_datum;
 }
 
 /* Returns true if 'row' is conceptually part of the database as modified by
@@ -202

[ovs-dev] [PATCHv3 3/4] ovsdb-idl: Avoid new expression.

[Joe Stringer]

In C++, 'new' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'new_datum' to
avoid this issue.

Signed-off-by: Joe Stringer <j...@ovn.org>
Acked-by: Ben Pfaff <b...@ovn.org>
---
v3: Add ack.
v2: Rebase.
Rename 'new_' to 'new_datum'.
Update comments above 'struct ovsdb_idl_row'.
---
 lib/ovsdb-data.h |   4 +-
 lib/ovsdb-idl-provider.h |  24 +--
 lib/ovsdb-idl.c  | 103 +--
 3 files changed, 68 insertions(+), 63 deletions(-)

diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index 1bf90d59c30f..756219e9f497 100644
--- a/lib/ovsdb-data.h
+++ b/lib/ovsdb-data.h
@@ -222,10 +222,10 @@ void ovsdb_datum_subtract(struct ovsdb_datum *a,
 /* Generate and apply diffs */
 void ovsdb_datum_diff(struct ovsdb_datum *diff,
   const struct ovsdb_datum *old,
-  const struct ovsdb_datum *new,
+  const struct ovsdb_datum *new_datum,
   const struct ovsdb_type *type);
 
-struct ovsdb_error *ovsdb_datum_apply_diff(struct ovsdb_datum *new,
+struct ovsdb_error *ovsdb_datum_apply_diff(struct ovsdb_datum *new_datum,
const struct ovsdb_datum *old,
const struct ovsdb_datum *diff,
const struct ovsdb_type *type)
diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index cdb41221e43d..ca6f58c9d763 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -37,30 +37,30 @@
  * - 'old' points to the data committed to the database and currently
  *   in the row.
  *
- * - 'new == old'.
+ * - 'new_datum == old'.
  *
  * When a transaction is in progress, the situation is a little different.  For
- * a row inserted in the transaction, 'old' is NULL and 'new' points to the
- * row's initial contents.  Otherwise:
+ * a row inserted in the transaction, 'old' is NULL and 'new_datum' points to
+ * the row's initial contents.  Otherwise:
  *
  * - 'old' points to the data committed to the database and currently in
  *   the row.  (This is the same as when no transaction is in progress.)
  *
- * - If the transaction does not modify the row, 'new == old'.
+ * - If the transaction does not modify the row, 'new_datum == old'.
  *
- * - If the transaction modifies the row, 'new' points to the modified
- *   data.
+ * - If the transaction modifies the row, 'new_datum' points to the
+ *   modified data.
  *
- * - If the transaction deletes the row, 'new' is NULL.
+ * - If the transaction deletes the row, 'new_datum' is NULL.
  *
  * Thus:
  *
  * - 'old' always points to committed data, except that it is NULL if the
  *   row is inserted within the current transaction.
  *
- * - 'new' always points to the newest, possibly uncommitted version of the
- *   row's data, except that it is NULL if the row is deleted within the
- *   current transaction.
+ * - 'new_datum' always points to the newest, possibly uncommitted version
+ *   of the row's data, except that it is NULL if the row is deleted within
+ *   the current transaction.
  */
 struct ovsdb_idl_row {
 struct hmap_node hmap_node; /* In struct ovsdb_idl_table's 'rows'. */
@@ -71,9 +71,9 @@ struct ovsdb_idl_row {
 struct ovsdb_datum *old;/* Committed data (null if orphaned). */
 
 /* Transactional data. */
-struct ovsdb_datum *new;/* Modified data (null to delete row). */
+struct ovsdb_datum *new_datum; /* Modified data (null to delete row). */
 unsigned long int *prereqs; /* Bitmap of columns to verify in "old". */
-unsigned long int *written; /* Bitmap of columns from "new" to write. */
+unsigned long int *written; /* Bitmap of "new_datum" columns to write. */
 struct hmap_node txn_node;  /* Node in ovsdb_idl_txn's list. */
 unsigned long int *map_op_written; /* Bitmap of columns pending map ops. */
 struct map_op_list **map_op_lists; /* Per-column map operations. */
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 2d0aa85ee311..a1c83622ddb7 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -775,11 +775,11 @@ ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
 const struct ovsdb_idl_row *row;
 HMAP_FOR_EACH (row, hmap_node, >rows) {
 size_t n_dsts = 0;
-if (row->new) {
+if (row->new_datum) {
 size_t n_columns = shash_count(>table->columns);
 for (size_t j = 0; j < n_columns; j++) {
 const struct ovsdb_type *type = >columns[j].type;
-const struct ovsdb_datum *datum = >new[j];
+const struct ovsdb_datum *d

[ovs-dev] [PATCHv3 2/4] ovsdb-idl: Avoid mutable type specifier.

[Joe Stringer]

In C++, 'mutable' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'is_mutable' to
avoid this issue.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
v3: Rename to 'is_mutable'.
v2: Rebase.
---
 lib/ovsdb-idl-provider.h | 2 +-
 lib/ovsdb-idl.c  | 2 +-
 ovsdb/ovsdb-idlc.in  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index a2eb8cac67d7..cdb41221e43d 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -89,7 +89,7 @@ struct ovsdb_idl_row {
 struct ovsdb_idl_column {
 char *name;
 struct ovsdb_type type;
-bool mutable;
+bool is_mutable;
 void (*parse)(struct ovsdb_idl_row *, const struct ovsdb_datum *);
 void (*unparse)(struct ovsdb_idl_row *);
 };
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 227aa5fbfcb2..2d0aa85ee311 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -2871,7 +2871,7 @@ bool
 ovsdb_idl_is_mutable(const struct ovsdb_idl_row *row,
  const struct ovsdb_idl_column *column)
 {
-return column->mutable || (row->new && !row->old);
+return column->is_mutable || (row->new && !row->old);
 }
 
 /* Returns false if 'row' was obtained from the IDL, true if it was initialized
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index bb07c21a80d0..24e86b772fbe 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -1268,7 +1268,7 @@ void
  .type = {
 %(type)s
  },
- .mutable = %(mutable)s,
+ .is_mutable = %(mutable)s,
  .parse = %(s)s_parse_%(c)s,
  .unparse = %(s)s_unparse_%(c)s,
 },\n""" % {'P': prefix.upper(),
-- 
2.13.3

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

[ovs-dev] [PATCHv3 1/4] ovsdb-idl: Avoid class declaration.

[Joe Stringer]

In C++, 'class' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'class_' to
avoid this issue.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
v3: Also update ovsdb/ovsdb-idlc.in.
v2: Rebase.
---
 lib/db-ctl-base.c|   4 +-
 lib/ovsdb-idl-provider.h |   2 +-
 lib/ovsdb-idl.c  | 154 +++
 ovsdb/ovsdb-idlc.in  |   4 +-
 4 files changed, 82 insertions(+), 82 deletions(-)

diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index d597c6c2af6f..9fec6fa0d59e 100644
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -635,7 +635,7 @@ check_mutable(const struct ovsdb_idl_row *row,
 {
 if (!ovsdb_idl_is_mutable(row, column)) {
 ctl_fatal("cannot modify read-only column %s in table %s",
-  column->name, row->table->class->name);
+  column->name, row->table->class_->name);
 }
 }
 
@@ -1715,7 +1715,7 @@ cmd_show_find_table_by_row(const struct ovsdb_idl_row 
*row)
 const struct cmd_show_table *show;
 
 for (show = cmd_show_tables; show->table; show++) {
-if (show->table == row->table->class) {
+if (show->table == row->table->class_) {
 return show;
 }
 }
diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index 59fb24015904..a2eb8cac67d7 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -104,7 +104,7 @@ struct ovsdb_idl_table_class {
 };
 
 struct ovsdb_idl_table {
-const struct ovsdb_idl_table_class *class;
+const struct ovsdb_idl_table_class *class_;
 unsigned char *modes;/* OVSDB_IDL_* bitmasks, indexed by column. */
 bool need_table; /* Monitor table even if no columns are selected
   * for replication. */
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index e916e940b652..227aa5fbfcb2 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -89,11 +89,11 @@ enum ovsdb_idl_state {
 };
 
 struct ovsdb_idl {
-const struct ovsdb_idl_class *class;
+const struct ovsdb_idl_class *class_;
 struct jsonrpc_session *session;
 struct uuid uuid;
 struct shash table_by_name; /* Contains "struct ovsdb_idl_table *"s.*/
-struct ovsdb_idl_table *tables; /* Array of ->class->n_tables elements. */
+struct ovsdb_idl_table *tables; /* Array of ->class_->n_tables elements. */
 unsigned int change_seqno;
 bool verify_write_only;
 
@@ -270,7 +270,7 @@ ovsdb_idl_create(const char *remote, const struct 
ovsdb_idl_class *class,
 : 0);
 
 idl = xzalloc(sizeof *idl);
-idl->class = class;
+idl->class_ = class;
 idl->session = jsonrpc_session_open(remote, retry);
 shash_init(>table_by_name);
 idl->tables = xmalloc(class->n_tables * sizeof *idl->tables);
@@ -280,7 +280,7 @@ ovsdb_idl_create(const char *remote, const struct 
ovsdb_idl_class *class,
 size_t j;
 
 shash_add_assert(>table_by_name, tc->name, table);
-table->class = tc;
+table->class_ = tc;
 table->modes = xmalloc(tc->n_columns);
 memset(table->modes, default_mode, tc->n_columns);
 table->need_table = false;
@@ -338,7 +338,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl)
 ovsdb_idl_clear(idl);
 jsonrpc_session_close(idl->session);
 
-for (i = 0; i < idl->class->n_tables; i++) {
+for (i = 0; i < idl->class_->n_tables; i++) {
 struct ovsdb_idl_table *table = >tables[i];
 ovsdb_idl_condition_destroy(>condition);
 ovsdb_idl_destroy_indexes(table);
@@ -363,7 +363,7 @@ ovsdb_idl_clear(struct ovsdb_idl *idl)
 bool changed = false;
 size_t i;
 
-for (i = 0; i < idl->class->n_tables; i++) {
+for (i = 0; i < idl->class_->n_tables; i++) {
 struct ovsdb_idl_table *table = >tables[i];
 struct ovsdb_idl_row *row, *next_row;
 
@@ -768,9 +768,9 @@ ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
 struct uuid *dsts = NULL;
 size_t allocated_dsts = 0;
 
-for (size_t i = 0; i < idl->class->n_tables; i++) {
+for (size_t i = 0; i < idl->class_->n_tables; i++) {
 const struct ovsdb_idl_table *table = >tables[i];
-const struct ovsdb_idl_table_class *class = table->class;
+const struct ovsdb_idl_table_class *class = table->class_;
 
 const struct ovsdb_idl_row *row;
 HMAP_FOR_EACH (row, hmap_node, >rows) {
@@ -794,16 +794,16 @@ ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
 dsts, _dsts)) {
 VLOG_ERR("unexpected arc from %s row "UUID_FMT" to %s "

[ovs-dev] [PATCHv3 0/4] Improve C++ support for OVSDB IDL.

[Joe Stringer]

In the OVSDB IDL, we use C++ keywords such as "new", "mutable", "class"
for variable and field names. This series updates these names so that
they don't conflict with the C++ keywords, which should improve the
ability to use the IDL from C++ code. This series focuses primarily on
code that exists in the tree; To address such problems for generated
field names that come from an OVSDB schema, a subsequent patch will
still be required. For instance, the ovs-vswitchd schema has a column
named "protected". This ends up as a field name in the generated
lib/vswitch-idl.[ch] code, which causes similar problems to those
addressed by this series.

This series was tested this far using the following diff, but since not
all of the compile errors are addressed yet - specifically those
mentioned above - I'm not submitting the below with this series:

diff --git a/include/openvswitch/automake.mk b/include/openvswitch/automake.mk
index eabeb1e971f3..aaab92b59d9d 100644
--- a/include/openvswitch/automake.mk
+++ b/include/openvswitch/automake.mk
@@ -31,7 +31,8 @@ openvswitchinclude_HEADERS = \
include/openvswitch/version.h \
include/openvswitch/vconn.h \
include/openvswitch/vlog.h \
-   include/openvswitch/nsh.h
+   include/openvswitch/nsh.h \
+   lib/vswitch-idl.h

 if HAVE_CXX
 # OVS does not use C++ itself, but it provides public header files
@@ -44,7 +45,7 @@ nodist_include_openvswitch_libcxxtest_la_SOURCES = 
include/openvswitch/cxxtest.c
 include/openvswitch/cxxtest.cc: include/openvswitch/automake.mk
$(AM_V_GEN)for header in $(openvswitchinclude_HEADERS); do  \
  echo $$header;\
-   done | sed 's,^include/\(.*\)$$,#include <\1>,' > $@
+   done | sed -e 's,^include/\(.*\)$$,#include <\1>,' -e 
's,^lib/\(.*\)$$,#include <\1>,'  > $@
 endif

 # OVS does not use C++ itself, but it provides public header files
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 9c674e7f5e80..d5562add4410 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -136,7 +136,11 @@ def printCIDLHeader(schemaFile):
 #include "ovsdb-data.h"
 #include "ovsdb-idl-provider.h"
 #include "smap.h"
-#include "uuid.h"''' % {'prefix': prefix.upper()})
+#include "uuid.h"
+
+#ifdef  __cplusplus
+extern "C" {
+#endif''' % {'prefix': prefix.upper()})

 for tableName, table in sorted(schema.tables.items()):
 structName = "%s%s" % (prefix, tableName.lower())
@@ -1050,6 +1054,10 @@ const char *
 {
 return "%s";
 }
+
+#ifdef  __cplusplus
+}
+#endif
 """ % (prefix, schema.version))
 
 


Joe Stringer (4):
  ovsdb-idl: Avoid class declaration.
  ovsdb-idl: Avoid mutable type specifier.
  ovsdb-idl: Avoid new expression.
  ovsdb-idl: Rename 'old' to 'old_datum'.

 lib/db-ctl-base.c|   4 +-
 lib/ovsdb-data.h |   8 +-
 lib/ovsdb-idl-provider.h |  42 +++
 lib/ovsdb-idl.c  | 307 ---
 ovsdb/ovsdb-idlc.in  |   6 +-
 5 files changed, 187 insertions(+), 180 deletions(-)

-- 
2.13.3

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

Re: [ovs-dev] [PATCHv2 3/4] ovsdb-idl: Avoid new expression.

[Joe Stringer]

On 10 August 2017 at 13:30, Ben Pfaff <b...@ovn.org> wrote:
> On Wed, Aug 09, 2017 at 03:27:41PM -0700, Joe Stringer wrote:
>> In C++, 'new' is a keyword. If this is used as the name for a field,
>> then C++ compilers can get confused about the context and fail to
>> compile references to such fields. Rename the field to 'new_datum' to
>> avoid this issue.
>>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>> v2: Rebase.
>> Rename 'new_' to 'new_datum'.
>> Update comments above 'struct ovsdb_idl_row'.
>
> Should we rename 'old' to 'old_datum' for consistency?
>
> Acked-by: Ben Pfaff <b...@ovn.org>

Turns out that Zhenyu pointed this out in v1, so yeah:)

I'll add these acks and respin the series.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 2/4] ovsdb-idl: Avoid mutable type specifier.

[Joe Stringer]

On 9 August 2017 at 20:54, Gao Zhenyu  wrote:
> How about mutable --> is_mutable ?

Thanks, I'll apply this change and respin.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2] netdev: Free ifidx mapping in netdev_ports_remove().

[Joe Stringer]

On 9 August 2017 at 17:18, Joe Stringer <j...@ovn.org> wrote:
> Previously, netdev_ports_insert() would allocate and insert an
> ifindex->odp_port mapping, but netdev_ports_remove() would never remove
> the mapping or free the mapping structure. This patch fixes these up.
>
> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
> Reported-by: Andy Zhou <az...@ovn.org>
> Signed-off-by: Joe Stringer <j...@ovn.org>
> Acked-by: Andy Zhou <az...@ovn.org>
> ---

Andy gave me an offline review of this, so I applied this incremental
and pushed the patch to master and branch-2.8:

diff --git a/lib/netdev.c b/lib/netdev.c
index 3cf9117b3683..b4e570bafd08 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -2272,12 +2272,13 @@ netdev_ports_remove(odp_port_t port_no, const
struct dpif_class *dpif_class)
 struct ifindex_to_port_data *ifidx = NULL;

 HMAP_FOR_EACH_WITH_HASH (ifidx, node, ifindex, _to_port) {
-if (ifidx && ifidx->port == port_no) {
+if (ifidx->port == port_no) {
 hmap_remove(_to_port, >node);
+free(ifidx);
 break;
 }
 }
-free(ifidx);
+ovs_assert(ifidx);
 } else {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] openvswitch: Remove unnecessary newlines from OVS_NLERR uses

[Joe Stringer]

On 11 August 2017 at 04:26, Joe Perches <j...@perches.com> wrote:
> OVS_NLERR already adds a newline so these just add blank
> lines to the logging.
>
> Signed-off-by: Joe Perches <j...@perches.com>

Acked-by: Joe Stringer <j...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 1/4] ovsdb-idl: Avoid class declaration.

[Joe Stringer]

On 10 August 2017 at 11:41, Ben Pfaff  wrote:
> On Thu, Aug 10, 2017 at 01:01:32PM +0800, Gao Zhenyu wrote:
>> How about:
>> struct ovsdb_idl_table {
>> ...
>> const struct ovsdb_idl_table_class *table_class
>> 
>> }
>>
>> struct ovsdb_idl {
>> 
>>  const struct ovsdb_idl_class *idl_class;
>> 
>
> Why make it longer?
>
>> Besides of that, I see many places consume the table class.
>> Do you mind to make a macro helps to fetch the class?
>> Like:
>>
>> #define OVSDB_GET_TABLE_CLASS(row) \
>> ((row)->table->class)
>
> Ugh.

Just in case the context isn't obvious... ;-)

I think that in general, we try not to obscure what's going on in OVS
code. This particular suggestion makes it less clear when reading the
code exactly where the class comes from, because the pointer
dereference is then hidden behind the macro. It also makes lines
referencing the class longer, and some of these lines are already
fairly long.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCHv2] netdev: Free ifidx mapping in netdev_ports_remove().

[Joe Stringer]

Previously, netdev_ports_insert() would allocate and insert an
ifindex->odp_port mapping, but netdev_ports_remove() would never remove
the mapping or free the mapping structure. This patch fixes these up.

Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
Reported-by: Andy Zhou <az...@ovn.org>
Signed-off-by: Joe Stringer <j...@ovn.org>
Acked-by: Andy Zhou <az...@ovn.org>
---
v2: Added more safety checks for ifindex, NULL pointers.
Added warning message if the ifindex can't be obtained.
---
 lib/netdev.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/lib/netdev.c b/lib/netdev.c
index cb4d9f007b85..3cf9117b3683 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -2266,6 +2266,26 @@ netdev_ports_remove(odp_port_t port_no, const struct 
dpif_class *dpif_class)
 data = netdev_ports_lookup(port_no, dpif_class);
 
 if (data) {
+int ifindex = netdev_get_ifindex(data->netdev);
+
+if (ifindex > 0) {
+struct ifindex_to_port_data *ifidx = NULL;
+
+HMAP_FOR_EACH_WITH_HASH (ifidx, node, ifindex, _to_port) {
+if (ifidx && ifidx->port == port_no) {
+hmap_remove(_to_port, >node);
+break;
+}
+}
+free(ifidx);
+} else {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+
+VLOG_WARN_RL(, "netdev ports map has dpif port %"PRIu32
+ " but netdev has no ifindex: %s", port_no,
+ ovs_strerror(ifindex));
+}
+
 dpif_port_destroy(>dpif_port);
 netdev_close(data->netdev); /* unref and possibly close */
 hmap_remove(_to_netdev, >node);
-- 
2.13.3

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

Re: [ovs-dev] [PATCH 2/3] checkpatch: Fix matching on C filenames.

[Joe Stringer]

On 9 August 2017 at 13:58, Ben Pfaff <b...@ovn.org> wrote:
> On Wed, Aug 09, 2017 at 01:37:51PM -0700, Joe Stringer wrote:
>> Most of the prerequisite checks so far matched on filenames that ended
>> in some character followed by 'c' or 'h', rather than a filename that
>> ends in '.c' or '.h'. Fix this.
>>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>
> Acked-by: Ben Pfaff <b...@ovn.org>

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/3] checkpatch: Check for infix operator whitespace.

[Joe Stringer]

On 9 August 2017 at 13:58, Ben Pfaff <b...@ovn.org> wrote:
> On Wed, Aug 09, 2017 at 01:37:50PM -0700, Joe Stringer wrote:
>> The 'Expressions' section of the coding style specifies that one space
>> should be on either side of infix binary and ternary operators. This
>> adds a check to checkpatch.py for most of these.
>>
>> The regex won't match if there are speech marks on the line, because
>> the style should not apply to the contents of strings.
>>
>> This check is left at warning level because there isn't a good way to
>> determine whether a line is within a multiline comment or string, so it
>> will occasionally flag such lines which contain hyphenated words.
>>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>
> Acked-by: Ben Pfaff <b...@ovn.org>

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH trivial 1/3] netdev-dummy: Fix minor style variation.

[Joe Stringer]

On 9 August 2017 at 13:46, Ben Pfaff <b...@ovn.org> wrote:
> On Wed, Aug 09, 2017 at 01:38:05PM -0700, Joe Stringer wrote:
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>
> For all three patches:
> Acked-by: Ben Pfaff <b...@ovn.org>

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv3 1/2] netdev: Free ifidx mapping in netdev_ports_remove().

[Joe Stringer]

On 8 August 2017 at 18:16, Andy Zhou <az...@ovn.org> wrote:
> On Tue, Aug 8, 2017 at 5:10 PM, Joe Stringer <j...@ovn.org> wrote:
>> Previously, netdev_ports_insert() would allocate and insert an
>> ifindex->odp_port mapping, but netdev_ports_remove() would never remove
>> the mapping or free the mapping structure. This patch fixes these up.
>>
>> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
>> Reported-by: Andy Zhou <az...@ovn.org>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>
> A few minor comments inline. Otherwise,
>
> Acked-by: Andy Zhou <az...@ovn.org>

Hi Andy,

Thanks for the review. Your comments are good points, and they change
the patch enough that I figured I should just send a v2. I'll follow
up shortly.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv3 2/2] dpif: Clean up netdev_ports map on dpif_close().

[Joe Stringer]

On 8 August 2017 at 18:33, Andy Zhou <az...@ovn.org> wrote:
> On Tue, Aug 8, 2017 at 5:10 PM, Joe Stringer <j...@ovn.org> wrote:
>> Commit 32b77c316d9982("dpif: Save added ports in a port map.")
>> introduced tracking of all dpif ports by taking a reference on each
>> available netdev when the dpif is opened, but it failed to clear out and
>> release references to these netdevs when the dpif is closed.
>>
>> One of the problems introduced by this was that upon clean exit of
>> ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device
>> was not deleted. This which could cause problems in subsequent start up.
>> Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed
>> this particular problem by not adding such devices to the netdev_ports
>> map, but the referencing/unreferencing upon dpif_open()/dpif_close() is
>> still not balanced.
>>
>> Balance the referencing of netdevs by clearing these during dpif_close().
>>
>> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>
> A minor comment below.
> Acked-by: Andy Zhou <az...@ovn.org>
>> ---
>> v3: Rather than flushing ports from this dpif, iterate ports and remove
>> them.
>> v2: Update commit message.
>> Rebase.
>> v1: Initial posting.
>> ---
>>  lib/dpif.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index e71f6a3d1475..eccd8607f17e 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -435,8 +435,17 @@ dpif_close(struct dpif *dpif)
>>  {
>>  if (dpif) {
>>  struct registered_dpif_class *rc;
>> +struct dpif_port_dump port_dump;
>> +struct dpif_port dpif_port;
>>
>>  rc = shash_find_data(_classes, dpif->dpif_class->type);
>> +
>> +DPIF_PORT_FOR_EACH (_port, _dump, dpif) {
>> +if (dpif_is_internal_port(dpif_port.type)) {
>> +continue;
>> +}
>> +netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
> Would it be easier to read with:
> if (!dpif_is_internal_port(dpif_port.type)) {
>  netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
>}

Yes, thanks for the suggestion. I applied this feedback and pushed the
patch to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCHv2 4/4] ovsdb-idl: Rename 'old' to 'old_datum'.

[Joe Stringer]

Now that the 'new' datum is named 'new_datum', be more consistent by
renaming 'old' to 'old_datum' to match.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
v2: New patch.
---
 lib/ovsdb-data.h |  4 +--
 lib/ovsdb-idl-provider.h | 22 +++---
 lib/ovsdb-idl.c  | 74 +---
 3 files changed, 51 insertions(+), 49 deletions(-)

diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index 756219e9f497..72c8fe35bce3 100644
--- a/lib/ovsdb-data.h
+++ b/lib/ovsdb-data.h
@@ -221,12 +221,12 @@ void ovsdb_datum_subtract(struct ovsdb_datum *a,
 
 /* Generate and apply diffs */
 void ovsdb_datum_diff(struct ovsdb_datum *diff,
-  const struct ovsdb_datum *old,
+  const struct ovsdb_datum *old_datum,
   const struct ovsdb_datum *new_datum,
   const struct ovsdb_type *type);
 
 struct ovsdb_error *ovsdb_datum_apply_diff(struct ovsdb_datum *new_datum,
-   const struct ovsdb_datum *old,
+   const struct ovsdb_datum *old_datum,
const struct ovsdb_datum *diff,
const struct ovsdb_type *type)
 OVS_WARN_UNUSED_RESULT;
diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index 94c604620833..22a67128736f 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -34,19 +34,19 @@
  *
  * When no transaction is in progress:
  *
- * - 'old' points to the data committed to the database and currently
+ * - 'old_datum' points to the data committed to the database and currently
  *   in the row.
  *
- * - 'new_datum == old'.
+ * - 'new_datum == old_datum'.
  *
  * When a transaction is in progress, the situation is a little different.  For
- * a row inserted in the transaction, 'old' is NULL and 'new_datum' points to
- * the row's initial contents.  Otherwise:
+ * a row inserted in the transaction, 'old_datum' is NULL and 'new_datum'
+ * points to the row's initial contents.  Otherwise:
  *
- * - 'old' points to the data committed to the database and currently in
- *   the row.  (This is the same as when no transaction is in progress.)
+ * - 'old_datum' points to the data committed to the database and currently
+ *   in the row.  (This is the same as when no transaction is in progress.)
  *
- * - If the transaction does not modify the row, 'new_datum == old'.
+ * - If the transaction does not modify the row, 'new_datum == old_datum'.
  *
  * - If the transaction modifies the row, 'new_datum' points to the
  *   modified data.
@@ -55,8 +55,8 @@
  *
  * Thus:
  *
- * - 'old' always points to committed data, except that it is NULL if the
- *   row is inserted within the current transaction.
+ * - 'old_datum' always points to committed data, except that it is NULL if
+ *   the row is inserted within the current transaction.
  *
  * - 'new_datum' always points to the newest, possibly uncommitted version
  *   of the row's data, except that it is NULL if the row is deleted within
@@ -68,11 +68,11 @@ struct ovsdb_idl_row {
 struct ovs_list src_arcs;   /* Forward arcs (ovsdb_idl_arc.src_node). */
 struct ovs_list dst_arcs;   /* Backward arcs (ovsdb_idl_arc.dst_node). */
 struct ovsdb_idl_table *table; /* Containing table. */
-struct ovsdb_datum *old;/* Committed data (null if orphaned). */
+struct ovsdb_datum *old_datum; /* Committed data (null if orphaned). */
 
 /* Transactional data. */
 struct ovsdb_datum *new_datum; /* Modified data (null to delete row). */
-unsigned long int *prereqs; /* Bitmap of columns to verify in "old". */
+unsigned long int *prereqs; /* Bitmap of "old_datum" columns to verify. */
 unsigned long int *written; /* Bitmap of "new_datum" columns to write. */
 struct hmap_node txn_node;  /* Node in ovsdb_idl_txn's list. */
 unsigned long int *map_op_written; /* Bitmap of columns pending map ops. */
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 7b244307ddd7..3eb4ecf15589 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1907,7 +1907,7 @@ ovsdb_idl_row_change__(struct ovsdb_idl_row *row, const 
struct json *row_json,
 }
 
 column_idx = column - table->class_->columns;
-old = >old[column_idx];
+old = >old_datum[column_idx];
 
 error = NULL;
 if (apply_diff) {
@@ -1992,7 +1992,7 @@ ovsdb_idl_row_apply_diff(struct ovsdb_idl_row *row,
 static bool
 ovsdb_idl_row_is_orphan(const struct ovsdb_idl_row *row)
 {
-return !row->old && !row->new_datum;
+return !row->old_datum && !row->new_datum;
 }
 
 /* Returns true if 'row' is conceptually part of the database as modified by
@@ -2025,7 +2025,7 @@ ovsdb_idl_row_parse(struct ovsdb_idl_row

[ovs-dev] [PATCHv2 3/4] ovsdb-idl: Avoid new expression.

[Joe Stringer]

In C++, 'new' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'new_datum' to
avoid this issue.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
v2: Rebase.
Rename 'new_' to 'new_datum'.
Update comments above 'struct ovsdb_idl_row'.
---
 lib/ovsdb-data.h |   4 +-
 lib/ovsdb-idl-provider.h |  24 +--
 lib/ovsdb-idl.c  | 103 +--
 3 files changed, 68 insertions(+), 63 deletions(-)

diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index 1bf90d59c30f..756219e9f497 100644
--- a/lib/ovsdb-data.h
+++ b/lib/ovsdb-data.h
@@ -222,10 +222,10 @@ void ovsdb_datum_subtract(struct ovsdb_datum *a,
 /* Generate and apply diffs */
 void ovsdb_datum_diff(struct ovsdb_datum *diff,
   const struct ovsdb_datum *old,
-  const struct ovsdb_datum *new,
+  const struct ovsdb_datum *new_datum,
   const struct ovsdb_type *type);
 
-struct ovsdb_error *ovsdb_datum_apply_diff(struct ovsdb_datum *new,
+struct ovsdb_error *ovsdb_datum_apply_diff(struct ovsdb_datum *new_datum,
const struct ovsdb_datum *old,
const struct ovsdb_datum *diff,
const struct ovsdb_type *type)
diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index aa1fa9390572..94c604620833 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -37,30 +37,30 @@
  * - 'old' points to the data committed to the database and currently
  *   in the row.
  *
- * - 'new == old'.
+ * - 'new_datum == old'.
  *
  * When a transaction is in progress, the situation is a little different.  For
- * a row inserted in the transaction, 'old' is NULL and 'new' points to the
- * row's initial contents.  Otherwise:
+ * a row inserted in the transaction, 'old' is NULL and 'new_datum' points to
+ * the row's initial contents.  Otherwise:
  *
  * - 'old' points to the data committed to the database and currently in
  *   the row.  (This is the same as when no transaction is in progress.)
  *
- * - If the transaction does not modify the row, 'new == old'.
+ * - If the transaction does not modify the row, 'new_datum == old'.
  *
- * - If the transaction modifies the row, 'new' points to the modified
- *   data.
+ * - If the transaction modifies the row, 'new_datum' points to the
+ *   modified data.
  *
- * - If the transaction deletes the row, 'new' is NULL.
+ * - If the transaction deletes the row, 'new_datum' is NULL.
  *
  * Thus:
  *
  * - 'old' always points to committed data, except that it is NULL if the
  *   row is inserted within the current transaction.
  *
- * - 'new' always points to the newest, possibly uncommitted version of the
- *   row's data, except that it is NULL if the row is deleted within the
- *   current transaction.
+ * - 'new_datum' always points to the newest, possibly uncommitted version
+ *   of the row's data, except that it is NULL if the row is deleted within
+ *   the current transaction.
  */
 struct ovsdb_idl_row {
 struct hmap_node hmap_node; /* In struct ovsdb_idl_table's 'rows'. */
@@ -71,9 +71,9 @@ struct ovsdb_idl_row {
 struct ovsdb_datum *old;/* Committed data (null if orphaned). */
 
 /* Transactional data. */
-struct ovsdb_datum *new;/* Modified data (null to delete row). */
+struct ovsdb_datum *new_datum; /* Modified data (null to delete row). */
 unsigned long int *prereqs; /* Bitmap of columns to verify in "old". */
-unsigned long int *written; /* Bitmap of columns from "new" to write. */
+unsigned long int *written; /* Bitmap of "new_datum" columns to write. */
 struct hmap_node txn_node;  /* Node in ovsdb_idl_txn's list. */
 unsigned long int *map_op_written; /* Bitmap of columns pending map ops. */
 struct map_op_list **map_op_lists; /* Per-column map operations. */
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index d474075fa990..7b244307ddd7 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -775,11 +775,11 @@ ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
 const struct ovsdb_idl_row *row;
 HMAP_FOR_EACH (row, hmap_node, >rows) {
 size_t n_dsts = 0;
-if (row->new) {
+if (row->new_datum) {
 size_t n_columns = shash_count(>table->columns);
 for (size_t j = 0; j < n_columns; j++) {
 const struct ovsdb_type *type = >columns[j].type;
-const struct ovsdb_datum *datum = >new[j];
+const struct ovsdb_datum *datum = >new_datum[j];
 add_row_refer

[ovs-dev] [PATCHv2 2/4] ovsdb-idl: Avoid mutable type specifier.

[Joe Stringer]

In C++, 'mutable' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'mutable_' to
avoid this issue.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
v2: Rebase.
---
 lib/ovsdb-idl-provider.h | 2 +-
 lib/ovsdb-idl.c  | 2 +-
 ovsdb/ovsdb-idlc.in  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index a2eb8cac67d7..aa1fa9390572 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -89,7 +89,7 @@ struct ovsdb_idl_row {
 struct ovsdb_idl_column {
 char *name;
 struct ovsdb_type type;
-bool mutable;
+bool mutable_;
 void (*parse)(struct ovsdb_idl_row *, const struct ovsdb_datum *);
 void (*unparse)(struct ovsdb_idl_row *);
 };
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 227aa5fbfcb2..d474075fa990 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -2871,7 +2871,7 @@ bool
 ovsdb_idl_is_mutable(const struct ovsdb_idl_row *row,
  const struct ovsdb_idl_column *column)
 {
-return column->mutable || (row->new && !row->old);
+return column->mutable_ || (row->new && !row->old);
 }
 
 /* Returns false if 'row' was obtained from the IDL, true if it was initialized
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index f065ef1c68fe..36b7d2700bb6 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -1268,7 +1268,7 @@ void
  .type = {
 %(type)s
  },
- .mutable = %(mutable)s,
+ .mutable_ = %(mutable)s,
  .parse = %(s)s_parse_%(c)s,
  .unparse = %(s)s_unparse_%(c)s,
 },\n""" % {'P': prefix.upper(),
-- 
2.13.3

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

[ovs-dev] [PATCHv2 1/4] ovsdb-idl: Avoid class declaration.

[Joe Stringer]

In C++, 'class' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'class_' to
avoid this issue.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
v2: Rebase.
---
 lib/db-ctl-base.c|   4 +-
 lib/ovsdb-idl-provider.h |   2 +-
 lib/ovsdb-idl.c  | 154 +++
 3 files changed, 80 insertions(+), 80 deletions(-)

diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index d597c6c2af6f..9fec6fa0d59e 100644
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -635,7 +635,7 @@ check_mutable(const struct ovsdb_idl_row *row,
 {
 if (!ovsdb_idl_is_mutable(row, column)) {
 ctl_fatal("cannot modify read-only column %s in table %s",
-  column->name, row->table->class->name);
+  column->name, row->table->class_->name);
 }
 }
 
@@ -1715,7 +1715,7 @@ cmd_show_find_table_by_row(const struct ovsdb_idl_row 
*row)
 const struct cmd_show_table *show;
 
 for (show = cmd_show_tables; show->table; show++) {
-if (show->table == row->table->class) {
+if (show->table == row->table->class_) {
 return show;
 }
 }
diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index 59fb24015904..a2eb8cac67d7 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -104,7 +104,7 @@ struct ovsdb_idl_table_class {
 };
 
 struct ovsdb_idl_table {
-const struct ovsdb_idl_table_class *class;
+const struct ovsdb_idl_table_class *class_;
 unsigned char *modes;/* OVSDB_IDL_* bitmasks, indexed by column. */
 bool need_table; /* Monitor table even if no columns are selected
   * for replication. */
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index e916e940b652..227aa5fbfcb2 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -89,11 +89,11 @@ enum ovsdb_idl_state {
 };
 
 struct ovsdb_idl {
-const struct ovsdb_idl_class *class;
+const struct ovsdb_idl_class *class_;
 struct jsonrpc_session *session;
 struct uuid uuid;
 struct shash table_by_name; /* Contains "struct ovsdb_idl_table *"s.*/
-struct ovsdb_idl_table *tables; /* Array of ->class->n_tables elements. */
+struct ovsdb_idl_table *tables; /* Array of ->class_->n_tables elements. */
 unsigned int change_seqno;
 bool verify_write_only;
 
@@ -270,7 +270,7 @@ ovsdb_idl_create(const char *remote, const struct 
ovsdb_idl_class *class,
 : 0);
 
 idl = xzalloc(sizeof *idl);
-idl->class = class;
+idl->class_ = class;
 idl->session = jsonrpc_session_open(remote, retry);
 shash_init(>table_by_name);
 idl->tables = xmalloc(class->n_tables * sizeof *idl->tables);
@@ -280,7 +280,7 @@ ovsdb_idl_create(const char *remote, const struct 
ovsdb_idl_class *class,
 size_t j;
 
 shash_add_assert(>table_by_name, tc->name, table);
-table->class = tc;
+table->class_ = tc;
 table->modes = xmalloc(tc->n_columns);
 memset(table->modes, default_mode, tc->n_columns);
 table->need_table = false;
@@ -338,7 +338,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl)
 ovsdb_idl_clear(idl);
 jsonrpc_session_close(idl->session);
 
-for (i = 0; i < idl->class->n_tables; i++) {
+for (i = 0; i < idl->class_->n_tables; i++) {
 struct ovsdb_idl_table *table = >tables[i];
 ovsdb_idl_condition_destroy(>condition);
 ovsdb_idl_destroy_indexes(table);
@@ -363,7 +363,7 @@ ovsdb_idl_clear(struct ovsdb_idl *idl)
 bool changed = false;
 size_t i;
 
-for (i = 0; i < idl->class->n_tables; i++) {
+for (i = 0; i < idl->class_->n_tables; i++) {
 struct ovsdb_idl_table *table = >tables[i];
 struct ovsdb_idl_row *row, *next_row;
 
@@ -768,9 +768,9 @@ ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
 struct uuid *dsts = NULL;
 size_t allocated_dsts = 0;
 
-for (size_t i = 0; i < idl->class->n_tables; i++) {
+for (size_t i = 0; i < idl->class_->n_tables; i++) {
 const struct ovsdb_idl_table *table = >tables[i];
-const struct ovsdb_idl_table_class *class = table->class;
+const struct ovsdb_idl_table_class *class = table->class_;
 
 const struct ovsdb_idl_row *row;
 HMAP_FOR_EACH (row, hmap_node, >rows) {
@@ -794,16 +794,16 @@ ovsdb_idl_check_consistency(const struct ovsdb_idl *idl)
 dsts, _dsts)) {
 VLOG_ERR("unexpected arc from %s row "UUID_FMT" to %s "
  "row "UUID_FMT,
-  

[ovs-dev] [PATCHv2 0/4] Improve C++ support for OVSDB IDL.

[Joe Stringer]

In the OVSDB IDL, we use C++ keywords such as "new", "mutable", "class"
for variable and field names. This series adds an underscore after each
usage of these names, to improve the ability to use the IDL from C++ code.
This series focuses primarily on code that exists in the tree; To
address such problems for generated field names that come from an OVSDB
schema, a subsequent patch will still be required. For instance, the
ovs-vswitchd schema has a column named "protected". This ends up as a
field name in the generated lib/vswitch-idl.[ch] code, which causes
similar problems to those addressed by this series.

This series was tested this far using the following diff, but since not
all of the compile errors are addressed yet - specifically those
mentioned above - I'm not submitting the below with this series:

diff --git a/include/openvswitch/automake.mk b/include/openvswitch/automake.mk
index 6bace61593ff..2bcf9c9ebedb 100644
--- a/include/openvswitch/automake.mk
+++ b/include/openvswitch/automake.mk
@@ -29,7 +29,8 @@ openvswitchinclude_HEADERS = \
include/openvswitch/uuid.h \
include/openvswitch/version.h \
include/openvswitch/vconn.h \
-   include/openvswitch/vlog.h
+   include/openvswitch/vlog.h \
+   lib/vswitch-idl.h

 if HAVE_CXX
 # OVS does not use C++ itself, but it provides public header files
@@ -42,7 +43,7 @@ nodist_include_openvswitch_libcxxtest_la_SOURCES = 
include/openvswitch/cxxtest.c
 include/openvswitch/cxxtest.cc: include/openvswitch/automake.mk
$(AM_V_GEN)for header in $(openvswitchinclude_HEADERS); do  \
  echo $$header;\
-   done | sed 's,^include/\(.*\)$$,#include <\1>,' > $@
+   done | sed -e 's,^include/\(.*\)$$,#include <\1>,' -e 
's,^lib/\(.*\)$$,#include <\1>,'  > $@
 endif

 # OVS does not use C++ itself, but it provides public header files
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 9c674e7f5e80..d5562add4410 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -136,7 +136,11 @@ def printCIDLHeader(schemaFile):
 #include "ovsdb-data.h"
 #include "ovsdb-idl-provider.h"
 #include "smap.h"
-#include "uuid.h"''' % {'prefix': prefix.upper()})
+#include "uuid.h"
+
+#ifdef  __cplusplus
+extern "C" {
+#endif''' % {'prefix': prefix.upper()})

 for tableName, table in sorted(schema.tables.items()):
 structName = "%s%s" % (prefix, tableName.lower())
@@ -1050,6 +1054,10 @@ const char *
 {
 return "%s";
 }
+
+#ifdef  __cplusplus
+}
+#endif
 """ % (prefix, schema.version))
 
 

Joe Stringer (4):
  ovsdb-idl: Avoid class declaration.
  ovsdb-idl: Avoid mutable type specifier.
  ovsdb-idl: Avoid new expression.
  ovsdb-idl: Rename 'old' to 'old_datum'.

 lib/db-ctl-base.c|   4 +-
 lib/ovsdb-data.h |   8 +-
 lib/ovsdb-idl-provider.h |  42 +++
 lib/ovsdb-idl.c  | 307 ---
 ovsdb/ovsdb-idlc.in  |   2 +-
 5 files changed, 185 insertions(+), 178 deletions(-)

-- 
2.13.3

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

[ovs-dev] [PATCH trivial 3/3] ovn-northd: Fix minor style variation.

[Joe Stringer]

Signed-off-by: Joe Stringer <j...@ovn.org>
---
 ovn/northd/ovn-northd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 99d15a7a56c4..49e4ac3383d3 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1886,7 +1886,7 @@ ovn_port_update_sbrec(struct northd_context *ctx,
 if (chassis) {
 /* If we found the chassis, and the gw chassis on record
  * differs from what we expect go ahead and update */
-if (op->sb->n_gateway_chassis !=1
+if (op->sb->n_gateway_chassis != 1
 || strcmp(op->sb->gateway_chassis[0]->chassis->name,
   chassis->name)
 || op->sb->gateway_chassis[0]->priority != 0) {
-- 
2.13.3

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

[ovs-dev] [PATCH trivial 1/3] netdev-dummy: Fix minor style variation.

[Joe Stringer]

Signed-off-by: Joe Stringer <j...@ovn.org>
---
 lib/netdev-dummy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 62ddd0c67834..f731af1dfd0e 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1574,7 +1574,7 @@ netdev_dummy_receive(struct unixctl_conn *conn,
 unixctl_command_reply_error(conn, "too small packet len");
 goto exit;
 }
-i+=2;
+i += 2;
 }
 /* Try parse 'argv[i]' as odp flow. */
 packet = eth_from_flow(flow_str, packet_size);
-- 
2.13.3

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

[ovs-dev] [PATCH 3/3] checkpatch: Check for trailing operators.

[Joe Stringer]

The style guide states that lines should not end with '?' or ':'. Check
for this and report an error.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
v2: Restrict to '?' and ':'.
Make sure that goto tags aren't flagged.
---
 utilities/checkpatch.py | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 1bb256ccd4d6..e553076d7d1c 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -94,6 +94,7 @@ __regex_ends_with_bracket = \
 re.compile(r'[^\s]\) {(\s+/\*[\s\Sa-zA-Z0-9\.,\?\*/+-]*)?$')
 __regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*[^*]')
 __regex_is_comment_line = re.compile(r'^\s*(/\*|\*\s)')
+__regex_trailing_operator = re.compile(r'^[^ ]* [^ ]*[?:]$')
 
 skip_leading_whitespace_check = False
 skip_trailing_whitespace_check = False
@@ -206,6 +207,11 @@ def is_comment_line(line):
 return __regex_is_comment_line.match(line) is not None
 
 
+def trailing_operator(line):
+"""Returns TRUE if the current line ends with an operator, eg &&"""
+return __regex_trailing_operator.match(line) is not None
+
+
 checks = [
 {'regex': None,
  'match_name':
@@ -237,7 +243,13 @@ checks = [
  'prereq': lambda x: not is_comment_line(x),
  'check': lambda x: pointer_whitespace_check(x),
  'print':
- lambda: print_error("Inappropriate spacing in pointer declaration")}
+ lambda: print_error("Inappropriate spacing in pointer declaration")},
+
+{'regex': '(\.c|\.h)(\.in)?$', 'match_name': None,
+ 'prereq': lambda x: not is_comment_line(x),
+ 'check': lambda x: trailing_operator(x),
+ 'print':
+ lambda: print_error("Line has '?' or ':' operator at end of line")},
 ]
 
 
-- 
2.13.3

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

[ovs-dev] [PATCH 1/3] checkpatch: Check for infix operator whitespace.

[Joe Stringer]

The 'Expressions' section of the coding style specifies that one space
should be on either side of infix binary and ternary operators. This
adds a check to checkpatch.py for most of these.

The regex won't match if there are speech marks on the line, because
the style should not apply to the contents of strings.

This check is left at warning level because there isn't a good way to
determine whether a line is within a multiline comment or string, so it
will occasionally flag such lines which contain hyphenated words.

Signed-off-by: Joe Stringer <j...@ovn.org>
---
 utilities/checkpatch.py | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 684c8b201d22..8b9ca0f504d3 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -275,6 +275,25 @@ checks += [
 for (function_name, description) in std_functions]
 
 
+def regex_operator_factory(operator):
+regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator)
+return lambda x: regex.search(x) is not None
+
+
+infix_operators = \
+[re.escape(op) for op in ['/', '%', '<<', '>>', '<=', '>=', '==', '!=',
+'^', '|', '&&', '||', '?:', '=', '+=', '-=', '*=', '/=', '%=',
+'&=', '^=', '|=', '<<=', '>>=']] \
++ ['[^<" ]<[^=" ]', '[^->" ]>[^=" ]', '[^ !()/"]\*[^/]', '[^ !&()"]&',
+   '[^" +(]\+[^"+;]', '[^" -(]-[^"->;]', '[^" <>=!^|+\-*/%&]=[^"=]']
+checks += [
+{'regex': '(\.c|\.h)(\.in)?$', 'match_name': None,
+ 'prereq': lambda x: not is_comment_line(x),
+ 'check': regex_operator_factory(operator),
+ 'print': lambda: print_warning("Line lacks whitespace around operator")}
+for operator in infix_operators]
+
+
 def get_file_type_checks(filename):
 """Returns the list of checks for a file based on matching the filename
against regex."""
-- 
2.13.3

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

Re: [ovs-dev] [patch_v4] dp-packet: Reset DPDK HWOL checksum flags on init.

[Joe Stringer]

On 8 August 2017 at 19:10, Darrell Ball  wrote:
> Thanks Joe
> I forgot to add your Tested-by to V5; I have been testing this myself; but 
> let me know if you would like it added – I can send a V6.

No worries, it looks like you'll resubmit with a slightly different
approach so feel free to drop these tested tags as they're no longer
applicable.

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

[ovs-dev] [PATCHv3 2/2] dpif: Clean up netdev_ports map on dpif_close().

[Joe Stringer]

Commit 32b77c316d9982("dpif: Save added ports in a port map.")
introduced tracking of all dpif ports by taking a reference on each
available netdev when the dpif is opened, but it failed to clear out and
release references to these netdevs when the dpif is closed.

One of the problems introduced by this was that upon clean exit of
ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device
was not deleted. This which could cause problems in subsequent start up.
Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed
this particular problem by not adding such devices to the netdev_ports
map, but the referencing/unreferencing upon dpif_open()/dpif_close() is
still not balanced.

Balance the referencing of netdevs by clearing these during dpif_close().

Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
Signed-off-by: Joe Stringer <j...@ovn.org>
---
v3: Rather than flushing ports from this dpif, iterate ports and remove
them.
v2: Update commit message.
Rebase.
v1: Initial posting.
---
 lib/dpif.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/lib/dpif.c b/lib/dpif.c
index e71f6a3d1475..eccd8607f17e 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -435,8 +435,17 @@ dpif_close(struct dpif *dpif)
 {
 if (dpif) {
 struct registered_dpif_class *rc;
+struct dpif_port_dump port_dump;
+struct dpif_port dpif_port;
 
 rc = shash_find_data(_classes, dpif->dpif_class->type);
+
+DPIF_PORT_FOR_EACH (_port, _dump, dpif) {
+if (dpif_is_internal_port(dpif_port.type)) {
+continue;
+}
+netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
+}
 dpif_uninit(dpif, true);
 dp_class_unref(rc);
 }
-- 
2.13.3

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

[ovs-dev] [PATCHv3 1/2] netdev: Free ifidx mapping in netdev_ports_remove().

[Joe Stringer]

Previously, netdev_ports_insert() would allocate and insert an
ifindex->odp_port mapping, but netdev_ports_remove() would never remove
the mapping or free the mapping structure. This patch fixes these up.

Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
Reported-by: Andy Zhou <az...@ovn.org>
Signed-off-by: Joe Stringer <j...@ovn.org>
---
v3: First post.
---
 lib/netdev.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/lib/netdev.c b/lib/netdev.c
index 7e9896b82928..a8f5348264c8 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -2266,9 +2266,20 @@ netdev_ports_remove(odp_port_t port_no, const struct 
dpif_class *dpif_class)
 data = netdev_ports_lookup(port_no, dpif_class);
 
 if (data) {
+int ifindex = netdev_get_ifindex(data->netdev);
+struct ifindex_to_port_data *ifidx;
+
+HMAP_FOR_EACH_WITH_HASH (ifidx, node, ifindex, _to_port) {
+if (ifidx->port == port_no) {
+break;
+}
+}
+
 dpif_port_destroy(>dpif_port);
 netdev_close(data->netdev); /* unref and possibly close */
+hmap_remove(_to_port, >node);
 hmap_remove(_to_netdev, >node);
+free(ifidx);
 free(data);
 ret = 0;
 }
-- 
2.13.3

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

Re: [ovs-dev] [PATCH 2/3] ovsdb-idl: Avoid new expression.

[Joe Stringer]

On 8 August 2017 at 00:46, Gao Zhenyu  wrote:
> Thanks for working on it!
>
> I  think new_ is not a good name. Could you please try to revise it?
> like: old --> old_datum
>new -->new_datum

Sure thing, that's a better name.

> BTW, you also need to update 'new' in the description of struct
> ovsdb_idl_row which in the top of lib/ovsdb-idl-provider.h

Good spotting. I'll wait a little while to see if there is any more
feedback, then respin the series to address these points.

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

Re: [ovs-dev] [patch_v4] dp-packet: Reset DPDK HWOL checksum flags on init.

[Joe Stringer]

On 8 August 2017 at 16:39, Darrell Ball <dlu...@gmail.com> wrote:
> Reset the DPDK HWOL checksum flags in dp_packet_init_.
> The new HWOL bad checksum flag is uninitialized on non-dpdk ports and
> this is noticed as test failures using netdev-dummy ports where the bad
> checksum flag is checked.
>
> Fixes: 7451af618e0d ("dp-packet : Update DPDK rx checksum validation 
> functions.")
> CC: Sugesh Chandran <sugesh.chand...@intel.com>
> Signed-off-by: Darrell Ball <dlu...@gmail.com>
> ---

Tested-by: Joe Stringer <j...@ovn.org>
Tested-at: https://travis-ci.org/joestringer/openvswitch/jobs/262464859

I'll let those more familiar with this code provide the review.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofp-print: #include its own header first.

[Joe Stringer]

On 8 August 2017 at 16:11, Ben Pfaff <b...@ovn.org> wrote:
> The OVS coding style document says that a .c file should include the
> corresponding .h file first, to ensure that the .h file includes all of
> its dependencies, but this file didn't do that.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---

Acked-by: Joe Stringer <j...@ovn.org>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] include: Add struct declaration to ofp-print.h.

[Joe Stringer]

On 8 August 2017 at 16:10, Ben Pfaff <b...@ovn.org> wrote:
> On Tue, Aug 08, 2017 at 02:30:28PM -0700, Joe Stringer wrote:
>> If a libopenvswitch user includes ofp-print.h before ofp-util.h (which
>> is standard alphabetical order), and turns on -Werror, then they would
>> hit this compilation error in the include:
>>
>> error: 'struct ofputil_port_map' declared inside parameter list will not
>> be visible outside of this definition or declaration [-Werror]
>>  void ofp_print(FILE *, const void *, size_t *, const struct 
>> ofputil_port_map *,
>>
>> Fixes: 50f96b10e1c8 ("Support accepting and displaying port names in OVS 
>> tools.")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>
> Our usual practice of having a .c file #include its own .h first should
> defend against this, but I see that it wasn't followed here.  I'll send
> a followup patch.
>
> Acked-by: Ben Pfaff <b...@ovn.org>

Thanks, applied to master and branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 2/2] dpif: Fix clean up of dpif_ports on dpif_close().

[Joe Stringer]

On 8 August 2017 at 13:48, Andy Zhou <az...@ovn.org> wrote:
> On Tue, Aug 8, 2017 at 11:23 AM, Joe Stringer <j...@ovn.org> wrote:
>> Commit 32b77c316d9982("dpif: Save added ports in a port map.")
>> introduced tracking of all dpif ports by taking a reference on each
>> available netdev when the dpif is opened, but it failed to clear out and
>> release references to these netdevs when the dpif is closed.
>>
>> One of the problems introduced by this was that upon clean exit of
>> ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device
>> was not deleted. This which could cause problems in subsequent start up.
>> Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed
>> this particular problem by not adding such devices to the netdev_ports
>> map, but the referencing/unreferencing upon dpif_open()/dpif_close() is
>> still not balanced.
>>
>> Balance the referencing of netdevs by introducing netdev_ports_flush()
>> and clearing these during dpif_close().
>>
>> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>
> I have a slightly different take on the bug fix. I am not super
> familiar with this code,
> so it could be wrong.
>
> Tracing the calling API, dpif_open -> do_open -> netdev_ports_insert().  So it
> would make sense for dpif_close to call netdev_ports_remove() to balance the
> reference issue mentioned in the comment. If true, then
> netdev_ports_flush() (previous
> patch) is not necessary.

I assume you mean netdev_port_data_destroy() ?

I see, rather than taking this flush() approach I could look at adding
DPIF_PORT_FOR_EACH() iteration code into the dpif_close(), which would
more closely match the do_open() code. Then it'd be more obvious that
these two paths are balanced.

> Currently, netdev_ports_remove() does not clean up ifindex_to_port
> map. Not clear
> it is correct.

You're right, and the ifidx is never freed either. I'll send a
followup series to address these comments.

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

[ovs-dev] [PATCH] include: Add struct declaration to ofp-print.h.

[Joe Stringer]

If a libopenvswitch user includes ofp-print.h before ofp-util.h (which
is standard alphabetical order), and turns on -Werror, then they would
hit this compilation error in the include:

error: 'struct ofputil_port_map' declared inside parameter list will not
be visible outside of this definition or declaration [-Werror]
 void ofp_print(FILE *, const void *, size_t *, const struct ofputil_port_map *,

Fixes: 50f96b10e1c8 ("Support accepting and displaying port names in OVS 
tools.")
Signed-off-by: Joe Stringer <j...@ovn.org>
---
 include/openvswitch/ofp-print.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/openvswitch/ofp-print.h b/include/openvswitch/ofp-print.h
index 20f049a37f65..d02634e3e91c 100644
--- a/include/openvswitch/ofp-print.h
+++ b/include/openvswitch/ofp-print.h
@@ -29,6 +29,7 @@ struct ofp10_match;
 struct ofp_flow_mod;
 struct ofp_header;
 struct ofputil_flow_stats;
+struct ofputil_port_map;
 struct ofputil_table_features;
 struct ofputil_table_stats;
 struct dp_packet;
-- 
2.13.3

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

[ovs-dev] [PATCHv2 2/2] dpif: Fix clean up of dpif_ports on dpif_close().

[Joe Stringer]

Commit 32b77c316d9982("dpif: Save added ports in a port map.")
introduced tracking of all dpif ports by taking a reference on each
available netdev when the dpif is opened, but it failed to clear out and
release references to these netdevs when the dpif is closed.

One of the problems introduced by this was that upon clean exit of
ovs-vswitchd via "ovs-appctl exit --cleanup", the "ovs-netdev" device
was not deleted. This which could cause problems in subsequent start up.
Commit 5119e258da92 ("dpif: Fix cleanup of userspace datapath.") fixed
this particular problem by not adding such devices to the netdev_ports
map, but the referencing/unreferencing upon dpif_open()/dpif_close() is
still not balanced.

Balance the referencing of netdevs by introducing netdev_ports_flush()
and clearing these during dpif_close().

Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
Signed-off-by: Joe Stringer <j...@ovn.org>
---
v2: Update commit message.
Rebase.
v1: Initial posting.
---
 lib/dpif.c   |  1 +
 lib/netdev.c | 15 +++
 lib/netdev.h |  1 +
 3 files changed, 17 insertions(+)

diff --git a/lib/dpif.c b/lib/dpif.c
index e71f6a3d1475..53bdf39f6e20 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -437,6 +437,7 @@ dpif_close(struct dpif *dpif)
 struct registered_dpif_class *rc;
 
 rc = shash_find_data(_classes, dpif->dpif_class->type);
+netdev_ports_flush(dpif->dpif_class);
 dpif_uninit(dpif, true);
 dp_class_unref(rc);
 }
diff --git a/lib/netdev.c b/lib/netdev.c
index 3e8b211857d7..94f9e486d8b1 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -2284,6 +2284,21 @@ netdev_ports_remove(odp_port_t port_no, const struct 
dpif_class *dpif_class)
 return ret;
 }
 
+void
+netdev_ports_flush(const struct dpif_class *class)
+{
+struct port_to_netdev_data *data, *next;
+
+ovs_mutex_lock(_hmap_mutex);
+HMAP_FOR_EACH_SAFE (data, next, node, _to_netdev) {
+if (data->dpif_class == class) {
+netdev_port_data_destroy(data);
+}
+}
+
+ovs_mutex_unlock(_hmap_mutex);
+}
+
 odp_port_t
 netdev_ifindex_to_odp_port(int ifindex)
 {
diff --git a/lib/netdev.h b/lib/netdev.h
index f8482f787e39..2d02be5826f6 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -215,6 +215,7 @@ int netdev_ports_insert(struct netdev *, const struct 
dpif_class *,
 struct dpif_port *);
 struct netdev *netdev_ports_get(odp_port_t port, const struct dpif_class *);
 int netdev_ports_remove(odp_port_t port, const struct dpif_class *);
+void netdev_ports_flush(const struct dpif_class *);
 odp_port_t netdev_ifindex_to_odp_port(int ifindex);
 struct netdev_flow_dump **netdev_ports_flow_dump_create(
 const struct dpif_class *,
-- 
2.13.3

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

Re: [ovs-dev] [PATCH 2/2] dpif: Fix clean up of dpif_ports on dpif_close().

[Joe Stringer]

On 7 August 2017 at 17:00, Ben Pfaff <b...@ovn.org> wrote:
> On Tue, Jun 27, 2017 at 11:13:10AM -0700, Joe Stringer wrote:
>> Commit 32b77c316d9982("dpif: Save added ports in a port map.")
>> introduced tracking of all dpif ports by taking a reference on each
>> available netdev when the dpif is opened, but it failed to clear out and
>> release references to these netdevs when the dpif is closed.
>>
>> Balance the referencing of netdevs by introducing netdev_ports_flush()
>> and clearing these during dpif_close().
>>
>> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>> CC: Paul Blakey <pa...@mellanox.com>
>> CC: Darrell Ball <dlu...@gmail.com>
>
> Hi Joe.  I'd like to take a look at this series but it no longer applies
> because it uses a macro DPIF_HMAP_KEY that no longer exists.  Would you
> mind rebasing and reposting?

Sure, I'll follow up with a rebase.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-kmod-macros: Load TFTP module.

[Joe Stringer]

On 7 August 2017 at 16:56, Andy Zhou <az...@ovn.org> wrote:
> On Mon, Aug 7, 2017 at 2:58 PM, Joe Stringer <j...@ovn.org> wrote:
>> Just like the FTP module needs to be loaded to ensure that the FTP tests
>> work, the TFTP module needs to be loaded to ensure that the TFTP tests
>> work. This patch does so.
>>
>> Fixes: 200a9af97d1c ("System tests: Add 4 new ftp and tftp tests.")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
> Acked-by: Andy Zhou <az...@ovn.org>

Thanks, applied to master and branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-traffic: Fix TFTP NAT skip check.

[Joe Stringer]

On 7 August 2017 at 13:35, Andy Zhou <az...@ovn.org> wrote:
> On Mon, Aug 7, 2017 at 1:05 PM, Joe Stringer <j...@ovn.org> wrote:
>> This test checked whether FTP support was available rather than TFTP.
>> It should check for TFTP, fix it.
>>
>> Fixes: 200a9af97d1c ("System tests: Add 4 new ftp and tftp tests.")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
> Acked-by: Andy Zhou <az...@ovn.org>

Thanks, applied to master and branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] system-kmod-macros: Load TFTP module.

[Joe Stringer]

Just like the FTP module needs to be loaded to ensure that the FTP tests
work, the TFTP module needs to be loaded to ensure that the TFTP tests
work. This patch does so.

Fixes: 200a9af97d1c ("System tests: Add 4 new ftp and tftp tests.")
Signed-off-by: Joe Stringer <j...@ovn.org>
---
 tests/system-kmod-macros.at | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
index e1b5707925a5..27498341a9f8 100644
--- a/tests/system-kmod-macros.at
+++ b/tests/system-kmod-macros.at
@@ -59,7 +59,8 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
 #
 m4_define([CHECK_CONNTRACK],
 [AT_SKIP_IF([test $HAVE_PYTHON = no])
- m4_foreach([mod], [[nf_conntrack_ipv4], [nf_conntrack_ipv6], 
[nf_nat_ftp]],
+ m4_foreach([mod], [[nf_conntrack_ipv4], [nf_conntrack_ipv6], [nf_nat_ftp],
+[nf_nat_tftp]],
 [modprobe mod || echo "Module mod not loaded."
  on_exit 'modprobe -r mod'
 ])
-- 
2.13.3

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

Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-upcall: Fix key attr iteration.

[Joe Stringer]

On 3 August 2017 at 14:35, Ben Pfaff <b...@ovn.org> wrote:
> On Mon, Jul 31, 2017 at 04:54:22PM -0700, Joe Stringer wrote:
>> This call is operating on messages generated by the datapath. If a
>> datapath implementation sends improperly formatted netlink attributes,
>> then it's possible for a revalidator thread to end up trapped in an
>> infinite loop iterating across these attributes. Rather than using the
>> UNSAFE variation of this iterator, use the regular version.
>>
>> Fixes: 994fcc5a15d3 ("upcall: Check for recirc_id in 
>> ukey_create_from_dpif_flow()")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>
> I wonder whether there's enough benefit in the _UNSAFE variants to keep
> them around.

Hmm. It's used in odp-execute to iterate generated actions, that's the
primary place that could have some sensitivity to this as it's used in
the userdp fastpath. If we can't measure an impact there with a decent
average actions list length, then perhaps it could be removed
altogether.

> Acked-by: Ben Pfaff <b...@ovn.org>

Thanks for the reviews, applied to master and branches 2.5-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] system-traffic: Fix TFTP NAT skip check.

[Joe Stringer]

This test checked whether FTP support was available rather than TFTP.
It should check for TFTP, fix it.

Fixes: 200a9af97d1c ("System tests: Add 4 new ftp and tftp tests.")
Signed-off-by: Joe Stringer <j...@ovn.org>
---
 tests/system-traffic.at | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 1522173f83a5..798dd2cbd2c2 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -3758,7 +3758,7 @@ OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
 AT_SETUP([conntrack - IPv4 TFTP with NAT])
-AT_SKIP_IF([test $HAVE_FTP = no])
+AT_SKIP_IF([test $HAVE_TFTP = no])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_NAT()
 CHECK_CONNTRACK_ALG()
-- 
2.13.3

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

Re: [ovs-dev] [PATCH 1/2] ofproto-dpif-upcall: Fix action attr iteration.

[Joe Stringer]

On 3 August 2017 at 14:34, Ben Pfaff <b...@ovn.org> wrote:
> On Mon, Jul 31, 2017 at 04:54:21PM -0700, Joe Stringer wrote:
>> This calls is operating on messages generated by the datapath. If a
>> datapath implementation sends improperly formatted netlink attributes,
>> then it's possible for a revalidator thread to end up trapped in an
>> infinite loop iterating across the actions attributes. Rather than using
>> the UNSAFE variation of this iterator, use the regular version.
>>
>> Fixes: e672ff9b4d22 ("ofproto-dpif: Restore metadata and registers on 
>> recirculation.")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>
> Acked-by: Ben Pfaff <b...@ovn.org>

Thanks for the reviews, applied to master and branches 2.5-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] ofproto-dpif-upcall: Fix action attr iteration.

[Joe Stringer]

On 2 August 2017 at 17:11, Greg Rose <gvrose8...@gmail.com> wrote:
> On 07/31/2017 04:54 PM, Joe Stringer wrote:
>>
>> This calls is operating on messages generated by the datapath. If a
>
>
> s/calls/call
>
>> datapath implementation sends improperly formatted netlink attributes,
>> then it's possible for a revalidator thread to end up trapped in an
>> infinite loop iterating across the actions attributes. Rather than using
>> the UNSAFE variation of this iterator, use the regular version.
>>
>> Fixes: e672ff9b4d22 ("ofproto-dpif: Restore metadata and registers on
>> recirculation.")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>>   ofproto/ofproto-dpif-upcall.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index b2f9d91d2d9c..8d1783accdc8 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1606,7 +1606,7 @@ ukey_create_from_dpif_flow(const struct udpif
>> *udpif,
>>   return EINVAL;
>>   }
>>   }
>> -NL_ATTR_FOR_EACH_UNSAFE (a, left, flow->actions, flow->actions_len) {
>> +NL_ATTR_FOR_EACH (a, left, flow->actions, flow->actions_len) {
>>   if (nl_attr_type(a) == OVS_ACTION_ATTR_RECIRC) {
>>   return EINVAL;
>>   }
>>
> I suppose we have to protect ourselves from malformed messages but I suppose
> there might be some small impact on performance?

Plausibly, but it's a bit hard to tell. I don't think it's big enough
that it outweighs making this code more robust.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev