OVS uses spaces for indentation in source code and it makes sense for it to
also use spaces for indentation in output. Spaces also consume less
horizontal space in output, which often makes it easier to read. This
commit transitions one part of output from tabs to spaces and updates
appropriate parts of the tests to match.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
I know of two reasons to mark a structure as "packed". The first is
because the structure must match some defined interface and therefore
compiler-inserted padding between or after members would cause its layout
to diverge from that interface. This is not a problem in a structure that
follows the general alignment rules that are seen in ABIs for all the
architectures that OVS cares about: basically, that a struct member needs
to be aligned on a boundary that is a multiple of the member's size.
The second reason is because instances of the struct tend to be at
misaligned addresses.
struct eth_header and struct vlan_eth_header are normally aligned on
16-bit boundaries (at least), and they contain only 16-bit members, so
there's no need to pack them. This commit removes the packed annotation.
This commit also removes the packed annotation from struct llc_header.
Since that struct only contains 8-bit members, I don't know of any benefit
to packing it, period.
This commit also removes a few more packed annotations that are much less
important.
When these packed annotations were removed, it caused a few warnings
related to casts from 'uint8_t *' to more strictly aligned pointer types,
related to struct ovs_action_push_tnl. That's because that struct had a
trailing member used to store packet headers, that was declared as
a uint8_t[]. Before, when this was cast to 'struct eth_header *', there
was no change in alignment since eth_header was packed; now that
eth_header is not packed, the compiler considers it suspicious. This
commit avoids that problem by changing the member from uint8_t[] to
uint32_t[], which assures the compiler that it is properly aligned.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Joe Stringer <joe@ovn.org>
The stp/show command will help users and developers to
get more details about stp. This patch works together with
the previous patch "stp: Change the api for next patch."
Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>
Co-authored-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Ben Pfaff <blp@ovn.org>
This patch changes the stp_port_get_role and removes
the stp_port_get_id, because stp/show has locked the
mutex before calling the stp_port_get_role, and
stp_port_get_id will not be used.
Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>
Acked-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Ben Pfaff <blp@ovn.org>
The stp/tcn command, which locks the mutex, was being registered without
initializing the mutex, so calling stp/tcn before STP was enabled on the
switch caused a crash. This commit fixes the bug by initializing the mutex
at the same time we register the stp/tcn command.
Reported-by: Ding Zhi <zhi.ding@6wind.com>
Reported-at: http://openvswitch.org/pipermail/dev/2016-May/071381.html
Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Quentin Monnet <quentin.monnet@6wind.com>
This attempts to prevent namespace collisions with other list libraries
Signed-off-by: Ben Warren <ben@skyportsystems.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Define struct eth_addr and use it instead of a uint8_t array for all
ethernet addresses in OVS userspace. The struct is always the right
size, and it can be assigned without an explicit memcpy, which makes
code more readable.
"struct eth_addr" is a good type name for this as many utility
functions are already named accordingly.
struct eth_addr can be accessed as bytes as well as ovs_be16's, which
makes the struct 16-bit aligned. All use seems to be 16-bit aligned,
so some algorithms on the ethernet addresses can be made a bit more
efficient making use of this fact.
As the struct fits into a register (in 64-bit systems) we pass it by
value when possible.
This patch also changes the few uses of Linux specific ETH_ALEN to
OVS's own ETH_ADDR_LEN, and removes the OFP_ETH_ALEN, as it is no
longer needed.
This work stemmed from a desire to make all struct flow members
assignable for unrelated exploration purposes. However, I think this
might be a nice code readability improvement by itself.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
In 'struct ofpbuf' the 'frame' pointer was used to parse different kinds of
data (Ethernet, OpenFlow, Netlink attributes). For Ethernet packets the
'frame' pointer was supposed to have the same value as the 'data'
pointer.
Since 'struct dp_packet' is only used for Ethernet packets, there's no
need for a separate 'frame' pointer: we can use the 'data' pointer
instead.
Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
Currently dp-packet make use of ofpbuf for managing packet
buffers. That complicates ofpbuf, by making dp-packet
independent of ofpbuf both libraries can be optimized for
their own use case.
This avoids mapping operation between ofpbuf and dp_packet
in datapath upcalls.
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
A new function vlog_insert_module() is introduced to avoid using
list_insert() from the vlog.h header.
Signed-off-by: Thomas Graf <tgraf@noironetworks.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Expose the struct ovs_list definition in <openvswitch/list.h>. Keep the
list access API private for now.
Signed-off-by: Thomas Graf <tgraf@noironetworks.com>
Acked-by: Ben Pfaff <blp@nicira.com>
struct list is a common name and can't be used in public headers.
Signed-off-by: Thomas Graf <tgraf@noironetworks.com>
Acked-by: Ben Pfaff <blp@nicira.com>
There is a difference between a port with STP/RSTP protocol enabled and a
disabled role and a port which has a disabled role because STP/RSTP is
not active. This commit ensure to make such distinction.
Standard 802.1D claims that the Topology Change state machine (17.31)
treats a Port as no longer active when it becomes an Alternate, Backup,
or Disabled Port and stops learning from received frames.
Signed-off-by: Daniele Venturino <daniele.venturino@m3s.it>
Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
Set the stp port name before enabling it, so that debugging messages
have the name to print out.
Do not treat the first state initialization as a state change. Zero
is not a valid state, so changing from zero to STP_DISABLED is not a
state change.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Acked-by: Daniele Venturino <daniele.venturino@m3s.it>
This commit adds more logging points in stp module for debugging.
Also, it makes the log print out the port name.
Signed-off-by: Alex Wang <alexw@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Commit 0d1cee123a84 (stp: Fix bpdu tx problem in listening state)
makes ovs drop the stp bpdu packets if stp is not enabled on the
input port. However, when pif bridge is used and stp is enabled
on the integration bridge. The flow translation of stp bpdu
packets will go through a level of resubmission which changes
the input port to the corresponding peer port. Since, the
patch port on the pif bridge does not have stp enabled, the
flow translation will drop the bpdu packets.
This commit fixes the issue by making ovs forward stp bpdu packets
on stp-disabled port.
VMware-BZ: #1284695
Signed-off-by: Alex Wang <alexw@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Acked-by: Joe Stringer <joestringer@nicira.com>
After a quick analysis, in most cases the access to refcounted objects
is clearly protected either with an explicit lock/mutex, or RCU. there
are only a few places where I left a call to ovs_refcount_unref().
Upon closer analysis it may well be that those could also use the
relaxed form.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Rename 'l2' to 'frame' and add new ofpbuf_set_frame() and ofpbuf_l2().
ofpbuf_set_frame() alse resets all the layer offsets. ofpbuf_l2()
returns NULL if the packet has no Ethernet header, as indicated either
by unset l3 offset or NULL frame pointer. Callers of ofpbuf_l2() are
supposed to check the return value, unless they can otherwise be sure
that the packet has a valid Ethernet header.
The recent commit 437d0d22 made some assumptions that were not valid
regarding the use of the 'l2' pointer in rconn module and by
compose_rarp(). This is now fixed as follows: rconn now relies on the
fact that once OpenFlow messages are given to rconn for transport, the
frame pointer is no longer needed to refer to the OpenFlow header; and
compose_rarp() now sets the frame pointer and offsets as expected.
In addition to storing network frames, ofpbufs are also used for
handling OpenFlow messages and action lists. lib/ofpbuf.h now has a
comment documenting the current usage conventions and invariants.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
This patch shrinks the struct ofpbuf from 104 to 48 bytes on 64-bit
systems, or from 52 to 36 bytes on 32-bit systems (counting in the
'l7' removal from an earlier patch). This may help contribute to
cache efficiency, and will speed up initializing, copying and
manipulating ofpbufs. This is potentially important for the DPDK
datapath, but the rest of the code base may also see a little benefit.
Changes are:
- Remove 'l7' pointer (previous patch).
- Use offsets instead of layer pointers for l2_5, l3, and l4 using
'l2' as basis. Usually 'data' is the same as 'l2', but this is not
always the case (e.g., when parsing or constructing a packet), so it
can not be easily used as the offset basis. Also, packet parsing is
faster if we do not need to maintain the offsets each time we pull
data from the ofpbuf.
- Use uint32_t for 'allocated' and 'size', as 2^32 is enough even for
largest possible messages/packets.
- Use packed enum for 'source'.
- Rearrange to avoid unnecessary padding.
- Remove 'private_p', which was used only in two cases, both of which
had the invariant ('l2' == 'data'), so we can temporarily use 'l2'
as a private pointer.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
The restriction only allows to send bpdu in forwarding state in
compose_output_action__. But a port could send bpdu in listening
and learning state according to comments in lib/stp.h(State of
an STP port).
Until this commit, OVS did not send out BPDUs in listening and learning
states. But those two states are temporary, the stp port will be in
forwarding state and send out BPDUs eventually (In the default
configuration listening and learning states last 15+15 second). Therefore,
this bug increased convergence time but did not entirely break STP.
Signed-off-by: kmindg <kmindg@gmail.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
None of the atomic implementations need a destroy function anymore, so it's
"more standard" and more convenient for users to get rid of them.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Andy Zhou <azhou@nicira.com>
This is a thin wrapper around an atomic_uint. It is useful anyhow because
each ovs_refcount_ref() or ovs_refcount_unref() call saves a few lines of
code.
This commit also changes all the potential direct users over to use the new
data structure.
Signed-off-by: Ben Pfaff <blp@nicira.com>
This allows other libraries to use util.h that has already
defined NOT_REACHED.
Signed-off-by: Harold Lim <haroldl@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Previously, as part of ofproto-dpif run() processing, we would loop
through all ports and poll for changes to carrier, bfd, cfm and lacp
status. This information is used to determine whether bundles may be
enabled, and to perform revalidation when needed.
This patch makes the bfd, cfm, lacp and stp modules aware of the new
global connectivity_seq, notifying on changes in port status. We can
then use connectivity_seq to check if anything has changed before
looping through all ports in ofproto-dpif. In a test environment of 5000
internal ports and 50 tunnel ports with bfd, this reduces average CPU
usage of the main thread from about 35% to about 25%.
Signed-off-by: Joe Stringer <joestringer@nicira.com>
Signed-off-by: Ethan Jackson <ethan@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>
The MSVC C library printf() implementation does not support the 'z', 't',
'j', or 'hh' format specifiers. This commit changes the Open vSwitch code
to avoid those format specifiers, switching to standard macros from
<inttypes.h> where available and inventing new macros resembling them
where necessary. It also updates CodingStyle to specify the macros' use
and adds a Makefile rule to report violations.
Signed-off-by: Alin Serdean <aserdean@cloudbasesolutions.com>
Co-authored-by: Ben Pfaff <blp@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
We've seen a number of deadlocks in the tree since thread safety was
introduced. So far, all of these are self-deadlocks, that is, a single
thread acquiring a lock and then attempting to re-acquire the same lock
recursively. When this has happened, the process simply hung, and it was
somewhat difficult to find the cause.
POSIX "error-checking" mutexes check for this specific problem (and
others). This commit switches from other types of mutexes to
error-checking mutexes everywhere that we can, that is, everywhere that
we're not using recursive mutexes. This ought to help find problems more
quickly in the future.
There might be performance advantages to other kinds of mutexes in some
cases. However, the existing mutex type choices were just guesses, so I'd
rather go for easy detection of errors until we know that other mutex
types actually perform better in specific cases. Also, I did a quick
microbenchmark of glibc mutex types on my host and found that the
error checking mutexes weren't any slower than the other types, at least
when the mutex is uncontended.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>
This commit changes the code such that arguments to thread-safety
macros are not ampersanded.
Signed-off-by: Alex Wang <alexw@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
This commit changes the code to use OVS_REQUIRES() instead of
OVS_REQ_WRLOCK(), for plain mutex.
Signed-off-by: Alex Wang <alexw@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
This is a straight search-and-replace, except that I also removed #include
<assert.h> from each file where there were no assert calls left.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>
Casts are sometimes necessary. One common reason that they are necessary
is for discarding a "const" qualifier. However, this can impede
maintenance: if the type of the expression being cast changes, then the
presence of the cast can hide a necessary change in the code that does the
cast. Using CONST_CAST, instead of a bare cast, makes these changes
visible.
Inspired by my own work elsewhere:
http://git.savannah.gnu.org/cgit/pspp.git/tree/src/libpspp/cast.h#n80
Signed-off-by: Ben Pfaff <blp@nicira.com>
Replaced all instances of Nicira Networks(, Inc) to Nicira, Inc.
Feature #10593
Signed-off-by: Raju Subramanian <rsubramanian@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
The unixctl library had used the vde2 management protocol since the
early days of Open vSwitch. As Open vSwitch has matured, several
Python daemons have been added to the code base which would benefit
from a unixctl implementations. Instead of implementing the old
unixctl protocol in Python, this patch changes unixctl to use JSON
RPC for which we already have an implementation in both Python and
C. Future patches will need to implement a unixctl library in
Python on top of JSON RPC.
Signed-off-by: Ethan Jackson <ethan@nicira.com>
Users may want to force the network to flush its MAC tables by
manually triggering a topology change event due to some event in
the system.
Signed-off-by: Ethan Jackson <ethan@nicira.com>
The spanning tree library keeps track of time to know how often it
should update its state and send out BPDUs. OVS is able to track time
in milliseconds, but STP uses a coarser-grained count (256 ticks per
second). To prevent losing milliseconds that didn't account for an
entire tick, the library keeps track of these remaining milliseconds. A
bug miscalculated the remainder and made it too high, which caused the
library to think time was passing more quickly than it was.
This bug wasn't noticeable on a quiet system, since STP only asks to be
woken every second. However, a system with a lot of activity would wake
OVS more frequently and have it call the subsystems' "run" functions.
Bug #8283
When debugging spanning tree, it's useful to have counters about how
many BPDUs have been sent and received. This commit keeps track of
these counters and stores them in a new "statistics" column of the Port
table.
Feature #8103
- Don't apply endian conversions to flags, which are 8 bits.
- Use #defines for default times for use outside library.
- Clarify our behavior when in STP_DISABLED state.
- Add "aux" member to STP port struct to be able to refer back to
the owning port.
- Define macros to print STP bridge and port ids.
- New helper function to get port id.
- New helper function to convert speed to cost.
- New functions to describe current role of port.
At one point, the OVS distribution contained an IEEE 802.1D Spanning
Tree Protocol (STP) library written by Ben Pfaff and based on the
802.1D-1998 reference code. It was never integrated into ovs-vswitchd,
so it was removed as part of commit ba18611 (Remove vestigial support
for Spanning Tree Protocol.)
This commit reintroduces the library, cleans up a few spots, and makes
it build cleanly against new code. A future commit will have
ovs-vswitchd use this library.
Open vSwitch has never properly supported IEEE 802.1D Spanning Tree
Protocol (STP), but it has various bits and pieces that claim to support
it. This commit deletes them, to reduce the amount of dead code in the
tree. We can always reintroduce it later if it proves to be a good idea.
Bug #1175.
Adding a macro to define the vlog module in use adds a level of
indirection, which makes it easier to change how the vlog module must be
defined. A followup commit needs to do that, so getting these widespread
changes out of the way first should make that commit easier to review.
On some system, at least, one must include <sys/types.h> before
<netinet/in.h>, and <netinet/in.h> before <arpa/inet.h> or <net/if.h>.
From Jean Tourrilhes <jt@hpl.hp.com>.
