[ovs-dev] [PATCH net 3/3] openvswitch: fix duplicate #include headers
The #include headers net/genetlink.h and linux/genetlink.h both were
included twice, so delete each of the duplicate.
Signed-off-by: Jean Sacren
Cc: Pravin Shelar
Cc: dev@openvswitch.org
---
net/openvswitch/datapath.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 7ad3f029baae..7228ec3faf19 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -31,40 +31,38 @@
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
-#include
-#include
#include
#include
#include
#include "datapath.h"
#include "flow.h"
#include "flow_table.h"
#include "flow_netlink.h"
#include "vport-internal_dev.h"
#include "vport-netdev.h"
int ovs_net_id __read_mostly;
static struct genl_family dp_packet_genl_family;
static struct genl_family dp_flow_genl_family;
static struct genl_family dp_datapath_genl_family;
static const struct genl_multicast_group ovs_dp_flow_multicast_group = {
.name = OVS_FLOW_MCGROUP,
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH branch-2.3] dpif-netdev: Avoid divide by zero.
No worries. This kind of thing happens on backports.
On Wed, Aug 6, 2014 at 6:27 PM, Andy Zhou wrote:
> Ben, thanks a lot for fixing it. Sorry I introduced the bug.
>
> On Wed, Aug 6, 2014 at 9:57 AM, Ben Pfaff wrote:
>> Thanks, applied to branch-2.3.
>>
>> On Wed, Aug 06, 2014 at 09:54:28AM -0700, Justin Pettit wrote:
>>> Acked-by: Justin Pettit
>>>
>>>
>>> On August 6, 2014 at 9:47:22 AM, Ben Pfaff (b...@nicira.com) wrote:
>>> > Otherwise creating the first dpif-netdev bridge fails because there are
>>> > no handlers:
>>> >
>>> > Program terminated with signal 8, Arithmetic exception.
>>> > #0 0x080971e9 in dp_execute_cb (aux_=aux_@entry=0xffcfaa54,
>>> > packet=packet@entry=0xffcfac54, md=md@entry=0xffcfac84,
>>> > a=a@entry=0x8f58930, may_steal=false) at ../lib/dpif-netdev.c:2154
>>> > #1 0x080b5adb in odp_execute_actions__ (dp=dp@entry=0xffcfaa54,
>>> > packet=packet@entry=0xffcfac54, steal=steal@entry=false,
>>> > md=md@entry=0xffcfac84, actions=actions@entry=0x8f58930,
>>> > actions_len=actions_len@entry=20,
>>> > dp_execute_action=dp_execute_action@entry=0x8097040 ,
>>> > more_actions=more_actions@entry=false) at ../lib/odp-execute.c:218
>>> > #2 0x080b5def in odp_execute_actions (dp=dp@entry=0xffcfaa54,
>>> > packet=packet@entry=0xffcfac54, steal=steal@entry=false,
>>> > md=md@entry=0xffcfac84, actions=0x8f58930, actions_len=20,
>>> > dp_execute_action=dp_execute_action@entry=0x8097040 )
>>> > at ../lib/odp-execute.c:285
>>> > #3 0x08095098 in dp_netdev_execute_actions (actions_len=,
>>> > actions=, md=0xffcfac84, may_steal=false,
>>> > packet=0xffcfac54, key=0xffcfaa5c, dp=)
>>> > at ../lib/dpif-netdev.c:2227
>>> > #4 dpif_netdev_execute (dpif=0x8f59598, execute=0xffcfac78)
>>> > at ../lib/dpif-netdev.c:1551
>>> > #5 0x0809a56c in dpif_execute (dpif=0x8f59598,
>>> > execute=execute@entry=0xffcfac78) at ../lib/dpif.c:1227
>>> > #6 0x08071071 in check_variable_length_userdata (backer=)
>>> > at ../ofproto/ofproto-dpif.c:1040
>>> > #7 open_dpif_backer (backerp=0x8f5834c, type=)
>>> > at ../ofproto/ofproto-dpif.c:921
>>> > #8 construct (ofproto_=0x8f581c0) at ../ofproto/ofproto-dpif.c:1120
>>> > #9 0x080675e0 in ofproto_create (datapath_name=0x8f57310 "br0",
>>> > datapath_type=, ofprotop=ofprotop@entry=0x8f576c8)
>>> > at ../ofproto/ofproto.c:564
>>> > #10 0x080529aa in bridge_reconfigure (ovs_cfg=ovs_cfg@entry=0x8f596d8)
>>> > at ../vswitchd/bridge.c:572
>>> > #11 0x08055e33 in bridge_run () at ../vswitchd/bridge.c:2339
>>> > #12 0x0804cdbd in main (argc=9, argv=0xffcfb554)
>>> > at ../vswitchd/ovs-vswitchd.c:116
>>> >
>>> > This bug was introduced by commit 9bcefb956d8f (ofproto-dpif: fix an ovs
>>> > crash when dpif_recv_set returns error).
>>> >
>>> > CC: Andy Zhou
>>> > Signed-off-by: Ben Pfaff
>>> > ---
>>> > This bug is only on branch-2.3.
>>> >
>>> > lib/dpif-netdev.c | 10 ++
>>> > 1 file changed, 6 insertions(+), 4 deletions(-)
>>> >
>>> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> > index 69e15d7..78f8636 100644
>>> > --- a/lib/dpif-netdev.c
>>> > +++ b/lib/dpif-netdev.c
>>> > @@ -2150,11 +2150,13 @@ dp_execute_cb(void *aux_, struct ofpbuf *packet,
>>> >
>>> > userdata = nl_attr_find_nested(a, OVS_USERSPACE_ATTR_USERDATA);
>>> >
>>> > - dp_netdev_output_userspace(aux->dp, packet,
>>> > - miniflow_hash_5tuple(aux->key, 0)
>>> > + if (aux->dp->n_handlers > 0) {
>>> > + dp_netdev_output_userspace(aux->dp, packet,
>>> > + miniflow_hash_5tuple(aux->key, 0)
>>> > % aux->dp->n_handlers,
>>> > - DPIF_UC_ACTION, aux->key,
>>> > - userdata);
>>> > + DPIF_UC_ACTION, aux->key,
>>> > + userdata);
>>> > + }
>>> >
>>> > if (may_steal) {
>>> > ofpbuf_delete(packet);
>>> > --
>>> > 1.7.10.4
>>> >
>>> > ___
>>> > dev mailing list
>>> > dev@openvswitch.org
>>> > http://openvswitch.org/mailman/listinfo/dev
>>> >
>>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
--
"I don't normally do acked-by's. I think it's my way of avoiding
getting blamed when it all blows up." Andrew Morton
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension)
Hello guys! I would also like to thank you for sharing with us your design in such a short period of time! I believe it is a great idea to have event registration for IOCTL (for notifications / waits). I have a few things that I would need some clarifications with: o) "Transaction based DPIF primitive": does this mean that we do Reads and Writes here? o) "State aware DPIF Dump commands" "Thus, the driver does not have to maintain any dump transaction outstanding request nor need to allocate any resources for it." You mean, whenever, say, a Flow dump request is issued, in one reply to give back all flows? o) "Event notification / NL multicast subscription" "An event (such as port addition/deletion link up/down) are propagated from the kernel to user mode through a subscription of a socket to a multicast group (nl_sock_join_mcgroup()) and a synchronous Receive (nl_sock_recv()) for retrieving the events" 1. I understand you do not speak of events here as API waitable / notification events, right? Otherwise, what is the format of the structs that would be read from nl_sock_recv()? 2. What would the relationship between hyper-v ports and hyper-v nics and dp ports would be? I mean, in the sense that the dp port additions and deletions would be requests coming from the userspace to the kernel (so no notification needed), while we get OIDs when nics connect & disconnect. In this sense, I see the hyper-v nic connection and disconnection as something that could be implemented as API notification events. o) "C. Implementation work flow" So our incremental development here would be: 1. Add a new device (alongside the existing one) 2. Implement a netlink protocol (for basic parsing attributes, etc.) for the new device 3. Implement netlink datapath operations for this device (get and dump only) 4. further & more advanced things are to be dealt with later. o) "One thing though is that, nl_sock_transact_multiple() might have to be modified to the series of nl_sock_send__() and nl_sock_recv__(), rather than doing a bunch of sends first and then doing the recvs. This is because Windows may not preserve message boundaries when we do the recv." If I understand what you mean, I think this is an implementation detail. Basically, for our driver, for unicast messages I know that we can do sequential reads. We hold an 'offset' in the buffer where the next read must begin from. However, as I remember, the implementation for "write" simply overwrites the previous buffer (of the corresponding socket). I believe it is good to keep one-write then one-receive instead of doing all writes, then all receives. However, I think we need to take into account the situation where the userspace might be providing a smaller buffer than it is the total to read. Also, I think the "dump" mechanism requires it. My suggestions & opinions: o) I think we must do dumping via writes and reads. The main reason is the fact that we don't know the total size to read when we request, say, a flow dump. o) I believe we shouldn't use the netlink overhead (nlmsghdr, genlmsghdr, attributes) when not needed (say, when registering a KEVENT notification) , and, if we choose not to use netlink protocol always, we may need a way to differentiate between netlink and non-netlink requests. Sam From: Eitan Eliahu [elia...@vmware.com] Sent: Wednesday, August 06, 2014 9:15 PM To: dev@openvswitch.org; Rajiv Krishnamurthy; Alin Serdean; Ben Pfaff; Kaushik Guha; Ben Pfaff; Justin Pettit; Nithin Raju; Alin Serdean; Ankur Sharma; Samuel Ghinet; Linda Sun; Keith Amidon Subject: Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension) Hello all, Here is a summary of our initial design. Not all areas are covered so we would be glad to discuss anything listed here and any other code/features we could leverage. Thanks! Eitan A. Objectives: [1] Create a NetLink (NL) driver interface for Windows which interoperates with the OVS NL user mode. [2] User mode code should be mostly cross platform with some minimal changes to support specific Windows OS calls. [3] The Driver should not have to maintain a state or resources for transaction or dumps [4] Reduce the number of system calls: User mode NL code should use Device IOCTL system call to send an NL commands and to receive the associated NL reply in the same system call, whenever possible (*). [5] An event may be associated with a NL socket I/O request to signal a completion for an outstanding receive operation on the socket. (For simplicity a single outstanding I/O request could be associated with a socket for the signaling purpose) (*) We assume Multiple NL transactions for the same socket can never be interleaved B. Netlink operation types: There are four types of interactions carried by processes through the NL layer: [1] Transa
Re: [ovs-dev] [PATCH RFC v2] lacp: Prefer slaves with running partner when selecting lead
On 05/08/14 22:16, Flavio Leitner wrote: On Mon, Aug 04, 2014 at 12:08:48PM -0700, Andy Zhou wrote: Zoltan, Sorry it took a while to get back to you. I am just coming up to speed on OVS LACP implementation, so my understanding may not be correct. Please feel free to point them out If I am wrong. According to wikipeida MC-LAG entry, there is no standard for it, they are mostly designed and implemented by vendors. After reading through the commit message, and comparing with the 802.1AX spec, I feel this seems like there is a bug in the MC-LAG implementation/configuration issue. When the partner on port A comes back again, should it wait for MC-LAG sync before using the default profile to exchange states with OVS? I agree that it sounds like a problem in the MC-LAG. However, I also agree that OVS could do better. The aggregation selection policy is somewhat a gray area not defined in any spec. The bonding driver offers ad_select= parameter which allows to switch to the new aggregator only if, for instance, all the ports are down in an active aggregator. The Team driver implementing 802.3ad also provides the policy selection parameter. The default is to consider the prio in the LACPDU, but you can also tell to not select any other aggregator if the current one is still usable, or per bandwidth or per number of ports available. My suggestion if we want to change something is to stick with bonding driver default behavior regarding to select a new aggregator: """ table or 0 The active aggregator is chosen by largest aggregate bandwidth. Reselection of the active aggregator occurs only when all slaves of the active aggregator are down or the active aggregator has no slaves. This is the default value. """ Documentation/networking/bonding.txt As far as I understood, there isn't really a concept of aggregator in OVS, but "lead" in lacp_update_attached() is something similar. However it is recalculated at every iteration, and it is simply just the slave with the lower priority. Do you mean we should save "lead" into a new field in struct lacp, and select a new one only if lacp_partner_ready(lead) == false? Zoli ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension)
Hi Sam, Here are some clarifications: >o) "Transaction based DPIF primitive": does this mean that we do Reads and >Writes here? Transaction based DPIF primitives are mapped into synchronous device I/O control system calls. The NL reply would be returned in the output buffer of the IOCTL parameter. >You mean, whenever, say, a Flow dump request is issued, in one reply to give >back all flows? Not necessarily. I meant that the driver does not have to maintain the state of the dump command. Each dump command sent down to the driver would be self-contained. >o) "Event notification / NL multicast subscription" >1. I understand you do not speak of events here as API waitable / notification >events, right? Yes, these are OVS events that are placed in a custom queue. There is a single Operating System event associated with the global socket which collects all OVS events. It will be triggered through a completion of a pending I/O request in the driver. > what is the format of the structs that would be read from nl_sock_recv()? The socket structure would contain a system Overlapped structure (along with an event). The Overlapped structure would be used only for unicast and multicast subscription. Transaction and dump based sockets will always not be waitable. >2. What would the relationship between hyper-v ports and hyper-v nics and dp >ports would be? >I mean, in the sense that the dp port additions and deletions would be >requests coming from the userspace to the kernel (so no notification needed), >while we get OIDs when nics connect & disconnect. In this sense, I see the >hyper-v nic connection and disconnection as something that could be >>implemented as API notification events. I assume that the above question is not related to the Netlink interface but I think your description is correct in general: Hyper-V ports (unlike tunnel ports) are created by the Hyper-V. The driver gets notified on every port creation or delition (or attribute change). In turn the driver queues an OVS event to a global queue (which was initially created when a multicat subscription IOCTL was sent to driver). Then, the driver will complete the pending IRP associated with the event queue. The user mode thread wairing on the event (associated with the Overlapped structure for this socket) will wake up and subsequently a DP port operation would be excuted. >o) "C. Implementation work flow" >So our incremental development here would be: >1. Add a new device (alongside the existing one) 2. Implement a netlink >protocol (for basic parsing attributes, etc.) for the new device 3. Implement >netlink datapath operations for this device (get and dump only) Yes >4. further & more advanced things are to be dealt with later. Event notification (multicast) and missed packet path (unicast) will be developed as a second phase. At this phase the FPID device object will be removed and the "new" vswitchd process will control the driver over the Netlink device interface >If I understand what you mean, I think this is an implementation detail. >Basically, for our driver, for unicast messages I know that we can do >sequential reads. We hold an 'offset' in the buffer where the next read must >begin from. However, as I remember, the implementation for "write" simply >overwrites the previous buffer (of the corresponding socket). I believe it is >good to >keep one-write then one-receive instead of doing all writes, then all >receives. >However, I think we need to take into account the situation where the >userspace might be providing a smaller buffer than it is the total to read. >Also, I think the "dump" mechanism requires it. I (want) to assume that each transaction is self-contained which means that the driver should not maintain a state of the transaction. Since, we will be using an IOCTL for that transaction the user mode buffer length will be specified in the command itself. All Write/Read dump pairs are replaced with a single IOCTL call. As I understand transactions and dump are (as used for DPIF) are not really socket operation per se. >My suggestions & opinions: >o) I think we must do dumping via writes and reads. The main reason is the >fact that we don't know the total size to read when we request, say, a flow >dump. /* Receive a reply. */ error = nl_sock_recv__(sock, buf_txn->reply, false); I am not familiar with ofpbuf structure. I noticed that you guys used MAX_STACK_LENGTH for specifying the buffer length. I need to get back to you on this one. >o) I believe we shouldn't use the netlink overhead (nlmsghdr, genlmsghdr, >attributes) when not needed (say, when registering a KEVENT notification) , >and, if w>e choose not to use netlink protocol always, we may need a way to >differentiate between netlink and non-netlink requests. Possible, as phase for optimization Thanks you Sam for reviewing these notes. Please feel free to ask or raise any comments. Eitan _
Re: [ovs-dev] [PATCH 1/2] datapath: Update comments about 'OVS_KEY_ATTR_8021Q'.
On Wed, Aug 6, 2014 at 2:55 PM, Justin Pettit wrote: > Commit fea393b1 (datapath: Describe policy for extending flow key, > implement needed changes.) changed the key 'OVS_KEY_ATTR_8021Q' to > 'OVS_KEY_ATTR_VLAN' and the size of the attribute structure. A couple > of comments were missed, so this commit updates them. > > Signed-off-by: Justin Pettit > --- > datapath/datapath.c |2 +- > lib/odp-util.h |2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/datapath/datapath.c b/datapath/datapath.c > index 1185f60..26d060e 100644 > --- a/datapath/datapath.c > +++ b/datapath/datapath.c > @@ -409,7 +409,7 @@ static size_t key_attr_size(void) > + nla_total_size(4) /* OVS_KEY_ATTR_RECIRC_ID */ > + nla_total_size(12) /* OVS_KEY_ATTR_ETHERNET */ > + nla_total_size(2) /* OVS_KEY_ATTR_ETHERTYPE */ > - + nla_total_size(4) /* OVS_KEY_ATTR_8021Q */ > + + nla_total_size(4) /* OVS_KEY_ATTR_VLAN */ > + nla_total_size(0) /* OVS_KEY_ATTR_ENCAP */ > + nla_total_size(2) /* OVS_KEY_ATTR_ETHERTYPE */ > + nla_total_size(40) /* OVS_KEY_ATTR_IPV6 */ > diff --git a/lib/odp-util.h b/lib/odp-util.h > index 82ab06d..a0c0ae8 100644 > --- a/lib/odp-util.h > +++ b/lib/odp-util.h > @@ -112,7 +112,7 @@ void odp_portno_names_destroy(struct hmap *portno_names); > * OVS_KEY_ATTR_RECIRC_ID 4-- 4 8 > * OVS_KEY_ATTR_ETHERNET 12-- 4 16 > * OVS_KEY_ATTR_ETHERTYPE 2 2 4 8 (outer VLAN > ethertype) > - * OVS_KEY_ATTR_8021Q 4-- 4 8 > + * OVS_KEY_ATTR_VLAN2 2 4 8 > * OVS_KEY_ATTR_ENCAP 0-- 4 4 (VLAN > encapsulation) > * OVS_KEY_ATTR_ETHERTYPE 2 2 4 8 (inner VLAN > ethertype) > * OVS_KEY_ATTR_IPV6 40-- 4 44 > -- LGTM. Acked-by: Pravin B Shelar ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] socket-util-unix: Fix umask race in bind_unix_socket().
On Thu, Jul 24, 2014 at 12:53:30PM -0700, Ben Pfaff wrote:
> The umask is a process-wide value, so bind_unix_socket() races with file
> creation in other Open vSwitch threads. This fixes the race.
>
> The workaround for non-Linux systems is not ideal, but I do not know any
> other general solution. I tested the workaround only on Linux.
The Linux part looks good. Although my style preference for
readability is like:
if (LINUX) {
/* On Linux, the fd's permissions become the file's permissions.
fchmod() does not affect other files, like umask() does. */
if (fchmod(fd, mode)) {
return errno;
}
/* must be after fchmod */
if (bind(fd, sun, sun_len))
return errno;
}
return 0;
}
I can't tell about the specifics of FreeBSD and NetBSD though
the work around looks sane too.
Acked-by: Flavio Leitner
>
> CC: YAMAMOTO Takashi
> Signed-off-by: Ben Pfaff
> ---
> lib/socket-util-unix.c | 35 ++-
> 1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/lib/socket-util-unix.c b/lib/socket-util-unix.c
> index 6b451d4..59b8160 100644
> --- a/lib/socket-util-unix.c
> +++ b/lib/socket-util-unix.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include "fatal-signal.h"
> #include "random.h"
> @@ -261,11 +262,35 @@ free_sockaddr_un(int dirfd, const char *linkname)
> static int
> bind_unix_socket(int fd, struct sockaddr *sun, socklen_t sun_len)
> {
> -/* According to _Unix Network Programming_, umask should affect bind().
> */
> -mode_t old_umask = umask(0077);
> -int error = bind(fd, sun, sun_len) ? errno : 0;
> -umask(old_umask);
> -return error;
> +const mode_t mode = 0700;
> +if (LINUX) {
> +/* On Linux, the fd's permissions become the file's permissions. */
> +return fchmod(fd, mode) || bind(fd, sun, sun_len) ? errno : 0;
> +} else {
> +/* On FreeBSD and NetBSD, only the umask affects permissions. The
> + * umask is process-wide rather than thread-specific, so we have to
> use
> + * a subprocess for safety. */
> +pid_t pid = fork();
> +
> +if (!pid) {
> +umask(mode ^ 0777);
> +_exit(bind(fd, sun, sun_len) ? errno : 0);
> +} else if (pid > 0) {
> +int status;
> +int error;
> +
> +do {
> +error = waitpid(pid, &status, 0) < 0 ? errno : 0;
> +} while (error == EINTR);
> +
> +return (error ? error
> +: WIFEXITED(status) ? WEXITSTATUS(status)
> +: WIFSIGNALED(status) ? EINTR
> +: ECHILD /* WTF? */);
> +} else {
> +return errno;
> +}
> +}
> }
>
> /* Creates a Unix domain socket in the given 'style' (either SOCK_DGRAM or
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel.
Hi Ankur, I would prefer to enhance odp-netlink.h if possible. One other idea that comes to mind is to create mockup files of the missing header files openvswitch.h(odp-netlink.h) is including and adding them to a directory. This directory can be used under the forwarding extension solution. This can help us in allowing us to put extra defines if needed in the future. Alin. -Mesaj original- De la: Ankur Sharma [mailto:ankursha...@vmware.com] Trimis: Thursday, August 7, 2014 7:11 AM Către: Alin Serdean; Nithin Raju Cc: Subiect: RE: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel. Hi Alin, Thanks a lot for the review. Comment: I was wondering why not generate the #ifdef over the file that already generates a copy of openvswitch.h (odp-netlink.h) needed for the extension? Reply: Yes sounds good. I did not update the original sed script as it would be full of ifdef based checks than, having a separate script looked cleaner to me. But i can make this change if you want to. == Comment: And as Nithin pointed out I think this can be solved in a more elegant solution. Reply: Yes, overall we discussed following approaches. 1. Have a seperate sed script to create odp-netlink for windows(submitted in patch). The script would be executed as a part ovs user space build, adhering to generation of odp-netlink.h. https://github.com/openvswitch/ovs/commit/837eefc76b3c79bb790a4c4c2d0a314d81b71a28 2. Same as above, but script will be executed as a part of windows driver build. 3. Create a static odp-netlink-windows.h and make it a part of git repo. We ruled out 3 as it would not automatically sync openvswitch.h. 2 is still under consideration as a long term solution but we did not do it as it would require that kernel build machine has sed as a powershell command (implying that mingw or some other package providing sed installation is a must on kernel build machines), also executing a pre build script as a part of vc project did not look straightforward when i tried it last time (for some other issue). Why we picked approach 1: a. It alligns with the odp-netlink.h generation. b. It will unblock the windows kernel build easily. c. After committing this patch i'll investigate approach 2 and implement that if everyone want to. Please let me know your thoughts, we can approach the issue accordingly. Regards, Ankur From: Alin Serdean Sent: Wednesday, August 6, 2014 6:17 PM To: Nithin Raju; Ankur Sharma Cc: Subject: RE: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a versionof odp-netlink for windows kernel. I was wondering why not generate the #ifdef over the file that already generates a copy of openvswitch.h (odp-netlink.h) needed for the extension? And as Nithin pointed out I think this can be solved in a more elegant solution. Thanks, Alin. -Mesaj original- De la: dev [mailto:dev-boun...@openvswitch.org] În numele Nithin Raju Trimis: Thursday, August 7, 2014 3:17 AM Către: Ankur Sharma Cc: Subiect: Re: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel. hi Ankur, This is not a perfect solution, but a step in the right direction of auto-generating a file for the Windows DP. Only comment I had is that we should get rid of definition of ETH_ALEN in OvsTypes.h. You can do that in another patch instead of re-spinning this one. Unless we change the approach, this patch looks good to me. Acked-by: Nithin Raju thanks, Nithin On Aug 6, 2014, at 4:30 PM, Ankur Sharma wrote: > odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel. > > Autogenerated odp-netlink.h will not compile with windows kernel, as > it refers to some userspace files like openvswitch/types.h and > packets.h which hyperv extension does not access. Due to this the > windows datapath compilation is broken on tip of tree. This patch > intends to fix that. > > In this patch we add a new sed script "extract-odp-netlink-windows-h" > to create odp-netlink-windows-dp.h. It works on similar lines as > extract-odp-netlink-h, but avoids including the header files which are > not available for driver. > > Also, added saurabh's fix to not to include some header files in > lib/netlink-protocol.h not needed by windows driver. > > After this fix, a userspace build will be needed before windows kernel > datapath can be built. > > Tested that hyperv extension could be built after building the > userspace. Verified vxlan tunnel based ping across hypervisors. > Verified that odp-netlink-windows-dp.h is not built for linux > platform. Ran 'make distcheck' to verify that nothing is broken on > linux. > > Signed-off-by: Ankur Sharma > Co-authored-by: Saurabh Shah > Tested-by: Ankur Sharma > Reported-by: Alin Serdean > Reported-by: Nithin Raju > Reported-at: > https://urldef
Re: [ovs-dev] Cloning packets for "action: set field"
Hello Saurabh, [QUOTE]For the set case, we actually send out the packet to all the existing output ports before going ahead with the set. This is what the function OvsOutputBeforeSetAction does. So, at any given point we only have one NBL to deal with.[/QUOTE] This may still not solve the problem: when the NdisFSendNetBufferLists function is called, the packet is being forwarded to the underlying drivers, and thus may spend some time until it is actually sent out. I believe this is an asynchronous operation (meaning that you just request a "send packet to other drivers" and may move forward before all is finished). I say this because I remember in my tests, it sometimes happens for packets to come in FilterSentNetBufferLists callback: NBL1, NBL2, NBL3 and NBL4, and only after NBL4 was finished being sent via NdisFSendNetBufferLists, FilterSentNetBufferListsComplete starts to be called. [QUOTE]We actually can copy parts of the header (even if they are in a single MDL), in fact our code has the infrastructure to do this. Please take a look at OvsPartialCopyNBL.[/QUOTE] I believe you missunderstand me: I was speaking about how NdisAllocateCloneNetBufferList and NdisRetreatNetBufferDataStart work. Both of them are being used by OvsPartialCopyNBL and the problem is not solved. The point with NdisAllocateCloneNetBufferList: "The clone NET_BUFFER_LIST structure describes the same data that is described by the NET_BUFFER_LIST structure at OriginalNetBufferList. NDIS does not copy the data that is described by the original MDLs to new data buffers. Instead, the cloned structures reference the original data buffers. " (http://msdn.microsoft.com/en-us/library/windows/hardware/ff560705%28v=vs.85%29.aspx) This means that by writing to the clone buffer you actually write to the original buffer. The buffer is REFERENCED, not a new one allocated. The point with NdisRetreatNetBufferDataStart: "If there isn't enough unused data space, this function allocates a new buffer and an MDL to describe the new buffer and chains the new MDL to the beginning of the MDL chain. " (http://msdn.microsoft.com/en-us/library/windows/hardware/ff564527%28v=vs.85%29.aspx) This means that if you have an NB with data offset = 40, you retreat by 40 and write 40 bytes from the "new beginning" of the buffer, you actually write to the same MDL. [QUOTE]The Buffer Management subsystem is one of the major difference between both the drivers. Since doing a deep copy of all packets is pretty sub-optimal, we (Guolin, in particular) designed the sophisticated buffer management system. Yes, it is intricate at first, but we already have it! (And for a while now. :))[/QUOTE] I particularly find it very intricate. Perhaps it should be refactored a bit and documented a bit more. For instance, functions such as OvsPartialCopyNBL and OvsPartialCopyToMultipleNBLs and OvsCopySinglePacketNBL and OvsFullCopyNBL and OvsFullCopyToMultipleNBLs and OvsCompleteNBL I find very daunting. Perhaps some comments for several parameters used and some comments in the function body would help as well. What does "Partial copy" actually mean? I understand full copy should mean something like "deep copy" / duplicate, but partial copy means create new NBL and new NB, and copy src buffer -> dst buffer ( = src buffer, because it's the same buffer)?? IMPORTANT: I have just made a test with your functionality from OvsPartialCopyNBL, and checked the cloned NBL against original NBL: original NBL: 0xe000`02e74b20 cloned NBL: 0xe000`02a53ac0 (ok) NBL's FirstNetBuffer original FirstNetBuffer: 0xe000`02e74c80; DataOffset = 0x26; DataLength = 0x5c cloned FirstNetBuffer: 0xe000`02fe4890; DataOffset = 0; DataLength = 0x5c (ok) CurrentMdl of the FirstNetBuffer original CurrentMdl: 0xe000`02e74da0; ByteCount = 0x50; (used space starting after 0x26 bytes, see above); Next MDL count bytes = 0x32; (0x50 - 0x26) + 0x32 = 0x5c cloned CurrentMdl: 0xe000`02e74dc6; ByteCount = 0x2a; Next MDL count bytes = 0x32; total size from MDLs = 0x5c (NOT OK -- see below) If you study CurrentMdl of both, you will see: original: is spanning from ...74da0 -> ...74DF0 (i.e. + 0x50), but excluding the unused area: (offset = 0x26): 74DC6 -> 74DF0 cloned: is spanning from ...74DC6 -> ...74DF0 (i.e. + 0x32). BOTH MDLS MAP THE SAME MEMORY AREA! So writing to cloneNb also writes to originalNb! Did you study the layout of NBLs, NBs and MDLs and buffers in your memory management functions? Sam From: Saurabh Shah [ssaur...@vmware.com] Sent: Wednesday, August 06, 2014 12:50 AM To: Samuel Ghinet Cc: dev@openvswitch.org Subject: RE: Cloning packets for "action: set field" Hi Samuel, > -Original Message- > From: Samuel Ghinet [mailto:sghi...@cloudbasesolutions.com] > Sent: Tuesday, August 05, 2014 5:00 AM > To: Saurabh Shah > Cc: dev@openvswitch.org > Subject: RE: Cloning packets for "action: set field" > >
Re: [ovs-dev] Cloning packets for "action: set field"
"it sometimes happens for packets to come in FilterSentNetBufferLists callback: NBL1, NBL2, NBL3 and NBL4, and only after NBL4 was finished being sent via NdisFSendNetBufferLists, FilterSentNetBufferListsComplete starts to be called." I mean, to enter the FilterSentNetBufferLists callback 4 times, each time for a NBL. And only after the 4th was done, the FilterSentNetBufferListsComplete callback to start being called. This may mean that it is possible that: 1) cloning NBL1 into NBL2 2) call NdisFSendNetBufferLists on NBL1 3) modify NBL2 may still corrupt the buffer of NBL1. Reading from MSDN: "As soon as a filter driver calls the NdisFSendNetBufferLists function, it relinquishes ownership of the NET_BUFFER_LIST structures and all associated resources. " "Until NDIS calls FilterSendNetBufferListsComplete, the current status of the send request is not available to the filter driver. A filter driver temporarily releases ownership of all resources that are associated with a send request when it calls NdisFSendNetBufferLists. A filter driver should never try to examine the NET_BUFFER_LIST structures or any associated data after calling NdisFSendNetBufferLists." (http://msdn.microsoft.com/en-us/library/windows/hardware/ff562616%28v=vs.85%29.aspx) From: Samuel Ghinet Sent: Thursday, August 07, 2014 8:39 PM To: Saurabh Shah Cc: dev@openvswitch.org Subject: RE: Cloning packets for "action: set field" Hello Saurabh, [QUOTE]For the set case, we actually send out the packet to all the existing output ports before going ahead with the set. This is what the function OvsOutputBeforeSetAction does. So, at any given point we only have one NBL to deal with.[/QUOTE] This may still not solve the problem: when the NdisFSendNetBufferLists function is called, the packet is being forwarded to the underlying drivers, and thus may spend some time until it is actually sent out. I believe this is an asynchronous operation (meaning that you just request a "send packet to other drivers" and may move forward before all is finished). I say this because I remember in my tests, it sometimes happens for packets to come in FilterSentNetBufferLists callback: NBL1, NBL2, NBL3 and NBL4, and only after NBL4 was finished being sent via NdisFSendNetBufferLists, FilterSentNetBufferListsComplete starts to be called. [QUOTE]We actually can copy parts of the header (even if they are in a single MDL), in fact our code has the infrastructure to do this. Please take a look at OvsPartialCopyNBL.[/QUOTE] I believe you missunderstand me: I was speaking about how NdisAllocateCloneNetBufferList and NdisRetreatNetBufferDataStart work. Both of them are being used by OvsPartialCopyNBL and the problem is not solved. The point with NdisAllocateCloneNetBufferList: "The clone NET_BUFFER_LIST structure describes the same data that is described by the NET_BUFFER_LIST structure at OriginalNetBufferList. NDIS does not copy the data that is described by the original MDLs to new data buffers. Instead, the cloned structures reference the original data buffers. " (http://msdn.microsoft.com/en-us/library/windows/hardware/ff560705%28v=vs.85%29.aspx) This means that by writing to the clone buffer you actually write to the original buffer. The buffer is REFERENCED, not a new one allocated. The point with NdisRetreatNetBufferDataStart: "If there isn't enough unused data space, this function allocates a new buffer and an MDL to describe the new buffer and chains the new MDL to the beginning of the MDL chain. " (http://msdn.microsoft.com/en-us/library/windows/hardware/ff564527%28v=vs.85%29.aspx) This means that if you have an NB with data offset = 40, you retreat by 40 and write 40 bytes from the "new beginning" of the buffer, you actually write to the same MDL. [QUOTE]The Buffer Management subsystem is one of the major difference between both the drivers. Since doing a deep copy of all packets is pretty sub-optimal, we (Guolin, in particular) designed the sophisticated buffer management system. Yes, it is intricate at first, but we already have it! (And for a while now. :))[/QUOTE] I particularly find it very intricate. Perhaps it should be refactored a bit and documented a bit more. For instance, functions such as OvsPartialCopyNBL and OvsPartialCopyToMultipleNBLs and OvsCopySinglePacketNBL and OvsFullCopyNBL and OvsFullCopyToMultipleNBLs and OvsCompleteNBL I find very daunting. Perhaps some comments for several parameters used and some comments in the function body would help as well. What does "Partial copy" actually mean? I understand full copy should mean something like "deep copy" / duplicate, but partial copy means create new NBL and new NB, and copy src buffer -> dst buffer ( = src buffer, because it's the same buffer)?? IMPORTANT: I have just made a test with your functionality from OvsPartialCopyNBL, and checked the cloned NBL against original NBL: original NBL: 0xe000`0
Re: [ovs-dev] Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension)
Hi Eithan, Do you have any particular reason to support both devices for start instead of focusing on the Netlink interface? On the patches progressing a bit slower than expected spent a bit too much time on the issue https://github.com/openvswitch/ovs-issues/issues/10 but I may have an idea which I would like to talk about in the next meeting. I plan to work in the weekend though to get so we can be one step close to our goal :). Alin. -Mesaj original- De la: Eitan Eliahu [mailto:elia...@vmware.com] Trimis: Thursday, August 7, 2014 3:19 AM Către: Alin Serdean; dev@openvswitch.org; Rajiv Krishnamurthy; Ben Pfaff; Kaushik Guha; Ben Pfaff; Justin Pettit; Nithin Raju; Ankur Sharma; Samuel Ghinet; Linda Sun; Keith Amidon Subiect: RE: Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension) Hi Alin, The driver which is currently checked in (the original one) supports the DPIF interface through a device object registered with the system. This driver works with a private version of user mode OVS (i.e. dpif-windows.c). The secondary device would be a second device object which supports the Nelink interface. For the initial development phase both devices will be instantiated and registered in the system. Thus, we could bring up all transaction and dump based DPIF commands over the Netlink device while the system is up and running. For clarity, let's call the "original device" the "DPIF device" and the "secondary device" the "Netlink device". Eitan -Original Message- From: Alin Serdean [mailto:aserd...@cloudbasesolutions.com] Sent: Wednesday, August 06, 2014 4:28 PM To: Eitan Eliahu; dev@openvswitch.org; Rajiv Krishnamurthy; Ben Pfaff; Kaushik Guha; Ben Pfaff; Justin Pettit; Nithin Raju; Ankur Sharma; Samuel Ghinet; Linda Sun; Keith Amidon Subject: RE: Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension) Hi Eitan, > C. Implementation work flow: > The driver creates a device object which provides a NetLink interface > for user mode processes. During the development phase this device is created > in addition to the existing DPIF device. (This means that the bring-up of the > NL based user mode can be done on a live kernel with resident DPs, ports and > flows) All transaction > and dump based DPIF functions could be developed and brought up when the NL > device is a secondary device (ovs-dpctl show and dump XXX should work). After >> the initial phase is completed (i.e. all transaction and dump based DPIF > primitives are implemented), the original device interface will be removed > and packet and > event propagation path will be brought up (driven by vswicth.exe) Could you, please explain a bit more what does original/secondary device mean? Ty! Alin. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension)
Alin, The two-device approach is for us to make sure that we are getting things correctly. Eg. what is the output of flow dump in one device, v/s the other device. I don't think any of the userspace code will have to worry about it. Like Eitan was mentioning in the previous mail, if we can get a ported dpif-linux.c, that should be sufficient for us. We'll use to this exercise the new NL device interface, and use the old dpif-windows.c to exercise the legacy interface. -- Nithin On Aug 7, 2014, at 10:49 AM, Alin Serdean wrote: > Hi Eithan, > > Do you have any particular reason to support both devices for start instead > of focusing on the Netlink interface? > > On the patches progressing a bit slower than expected spent a bit too much > time on the issue > https://urldefense.proofpoint.com/v1/url?u=https://github.com/openvswitch/ovs-issues/issues/10&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=ubrOpIWavCMqX4l4j1LEVpTfDj%2FD5Qyn8KCoJIBGvzo%3D%0A&m=z%2BzFYft67OyIMrbjU76jo6%2Bc7Pf%2F9dQp3vXZYkrIDKY%3D%0A&s=cf4b0dc3b9e800c895b90c3b41a50c6f3fa68f9f06f5c82c3f742b3d1d5810de > but I may have an idea which I would like to talk about in the next meeting. > I plan to work in the weekend though to get so we can be one step close to > our goal :). > > Alin. > > -Mesaj original- > De la: Eitan Eliahu [mailto:elia...@vmware.com] > Trimis: Thursday, August 7, 2014 3:19 AM > Către: Alin Serdean; dev@openvswitch.org; Rajiv Krishnamurthy; Ben Pfaff; > Kaushik Guha; Ben Pfaff; Justin Pettit; Nithin Raju; Ankur Sharma; Samuel > Ghinet; Linda Sun; Keith Amidon > Subiect: RE: Design notes for provisioning Netlink interface from the OVS > Windows driver (Switch extension) > > > Hi Alin, > The driver which is currently checked in (the original one) supports the DPIF > interface through a device object registered with the system. This driver > works with a private version of user mode OVS (i.e. dpif-windows.c). The > secondary device would be a second device object which supports the Nelink > interface. For the initial development phase both devices will be > instantiated and registered in the system. Thus, we could bring up all > transaction and dump based DPIF commands over the Netlink device while the > system is up and running. > > For clarity, let's call the "original device" the "DPIF device" and the > "secondary device" the "Netlink device". > Eitan > > -Original Message- > From: Alin Serdean [mailto:aserd...@cloudbasesolutions.com] > Sent: Wednesday, August 06, 2014 4:28 PM > To: Eitan Eliahu; dev@openvswitch.org; Rajiv Krishnamurthy; Ben Pfaff; > Kaushik Guha; Ben Pfaff; Justin Pettit; Nithin Raju; Ankur Sharma; Samuel > Ghinet; Linda Sun; Keith Amidon > Subject: RE: Design notes for provisioning Netlink interface from the OVS > Windows driver (Switch extension) > > Hi Eitan, > >> C. Implementation work flow: >> The driver creates a device object which provides a NetLink interface >> for user mode processes. During the development phase this device is created >> in addition to the existing DPIF device. (This means that the bring-up of >> the NL based user mode can be done on a live kernel with resident DPs, ports >> and flows) All transaction >> and dump based DPIF functions could be developed and brought up when the NL >> device is a secondary device (ovs-dpctl show and dump XXX should work). >> After> the initial phase is completed (i.e. all transaction and dump >> based DPIF primitives are implemented), the original device interface will >> be removed and packet and >> event propagation path will be brought up (driven by vswicth.exe) > > Could you, please explain a bit more what does original/secondary device mean? > > Ty! > Alin. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension)
Hi Alin, yes, we want to exercise the interface when OVS is running. For example we would like to dump the flow table is not empty. On the other issue (NBL with multiple NBs, Github issue #10) I think we need to talk how to support it. After you came across this issue we even know how to produce this case :-) Thanks, Eitan -Original Message- From: Alin Serdean [mailto:aserd...@cloudbasesolutions.com] Sent: Thursday, August 07, 2014 10:50 AM To: Eitan Eliahu; dev@openvswitch.org; Rajiv Krishnamurthy; Ben Pfaff; Kaushik Guha; Ben Pfaff; Justin Pettit; Nithin Raju; Ankur Sharma; Samuel Ghinet; Linda Sun; Keith Amidon Subject: RE: Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension) Hi Eithan, Do you have any particular reason to support both devices for start instead of focusing on the Netlink interface? On the patches progressing a bit slower than expected spent a bit too much time on the issue https://urldefense.proofpoint.com/v1/url?u=https://github.com/openvswitch/ovs-issues/issues/10&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=EXUTxzeugqhErQ2Fi%2BVBW0vsf89O8ECLmGTTV1lNZX0%3D%0A&s=70afc8a789fbcf2c754809a1f9d1246227250b279dd35e740bbb31b34544bc6b but I may have an idea which I would like to talk about in the next meeting. I plan to work in the weekend though to get so we can be one step close to our goal :). Alin. -Mesaj original- De la: Eitan Eliahu [mailto:elia...@vmware.com] Trimis: Thursday, August 7, 2014 3:19 AM Către: Alin Serdean; dev@openvswitch.org; Rajiv Krishnamurthy; Ben Pfaff; Kaushik Guha; Ben Pfaff; Justin Pettit; Nithin Raju; Ankur Sharma; Samuel Ghinet; Linda Sun; Keith Amidon Subiect: RE: Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension) Hi Alin, The driver which is currently checked in (the original one) supports the DPIF interface through a device object registered with the system. This driver works with a private version of user mode OVS (i.e. dpif-windows.c). The secondary device would be a second device object which supports the Nelink interface. For the initial development phase both devices will be instantiated and registered in the system. Thus, we could bring up all transaction and dump based DPIF commands over the Netlink device while the system is up and running. For clarity, let's call the "original device" the "DPIF device" and the "secondary device" the "Netlink device". Eitan -Original Message- From: Alin Serdean [mailto:aserd...@cloudbasesolutions.com] Sent: Wednesday, August 06, 2014 4:28 PM To: Eitan Eliahu; dev@openvswitch.org; Rajiv Krishnamurthy; Ben Pfaff; Kaushik Guha; Ben Pfaff; Justin Pettit; Nithin Raju; Ankur Sharma; Samuel Ghinet; Linda Sun; Keith Amidon Subject: RE: Design notes for provisioning Netlink interface from the OVS Windows driver (Switch extension) Hi Eitan, > C. Implementation work flow: > The driver creates a device object which provides a NetLink interface > for user mode processes. During the development phase this device is created > in addition to the existing DPIF device. (This means that the bring-up of the > NL based user mode can be done on a live kernel with resident DPs, ports and > flows) All transaction > and dump based DPIF functions could be developed and brought up when the NL > device is a secondary device (ovs-dpctl show and dump XXX should work). After >> the initial phase is completed (i.e. all transaction and dump based DPIF > primitives are implemented), the original device interface will be removed > and packet and > event propagation path will be brought up (driven by vswicth.exe) Could you, please explain a bit more what does original/secondary device mean? Ty! Alin. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel.
Hi Alin, Thanks a lot for your reply. == [COMMENT]: I would prefer to enhance odp-netlink.h if possible. [REPLY]: Sure I had a discussion on this with nithin. I'll talk to ben and confirm if he is ok with having ifdefs in odp-netlink.h. We'll go with one odp-netlink.h as long as ben is fine with it. === [COMMENT]: One other idea that comes to mind is to create mockup files of the missing header files openvswitch.h(odp-netlink.h) is including and adding them to a directory. This directory can be used under the forwarding extension solution. This can help us in allowing us to put extra defines if needed in the future. [REPLY]: Thanks a lot for proposing an alternate approach. I can think over it but i am not sure if it is going to give us something much better then what we plan to do. Having said that i would like to think over it more and then we can make a call. Meanwhile if you are ok then i would to submit a V2 of the patch with comment 1 incorporated and we can check it in. Comment 2 can be thought over and taken care of later. === Thanks. Regards, Ankur From: Alin Serdean Sent: Thursday, August 7, 2014 10:38 AM To: Ankur Sharma; Nithin Raju Cc: Subject: RE: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a versionof odp-netlink for windows kernel. Hi Ankur, I would prefer to enhance odp-netlink.h if possible. One other idea that comes to mind is to create mockup files of the missing header files openvswitch.h(odp-netlink.h) is including and adding them to a directory. This directory can be used under the forwarding extension solution. This can help us in allowing us to put extra defines if needed in the future. Alin. -Mesaj original- De la: Ankur Sharma [mailto:ankursha...@vmware.com] Trimis: Thursday, August 7, 2014 7:11 AM Către: Alin Serdean; Nithin Raju Cc: Subiect: RE: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel. Hi Alin, Thanks a lot for the review. Comment: I was wondering why not generate the #ifdef over the file that already generates a copy of openvswitch.h (odp-netlink.h) needed for the extension? Reply: Yes sounds good. I did not update the original sed script as it would be full of ifdef based checks than, having a separate script looked cleaner to me. But i can make this change if you want to. == Comment: And as Nithin pointed out I think this can be solved in a more elegant solution. Reply: Yes, overall we discussed following approaches. 1. Have a seperate sed script to create odp-netlink for windows(submitted in patch). The script would be executed as a part ovs user space build, adhering to generation of odp-netlink.h. https://urldefense.proofpoint.com/v1/url?u=https://github.com/openvswitch/ovs/commit/837eefc76b3c79bb790a4c4c2d0a314d81b71a28&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=f6EhnZ0ORGZNt5QbYmRaOxfWfx%2Bqd3KEiPf3%2FYaollU%3D%0A&m=6guioO%2FCDHM51PWGnmLdFqKKVNr06663Pzst8zz1ex4%3D%0A&s=5c4a7e31d3fff03c2332c004885ef37fd0392a5c6d3c7740367101441346433b 2. Same as above, but script will be executed as a part of windows driver build. 3. Create a static odp-netlink-windows.h and make it a part of git repo. We ruled out 3 as it would not automatically sync openvswitch.h. 2 is still under consideration as a long term solution but we did not do it as it would require that kernel build machine has sed as a powershell command (implying that mingw or some other package providing sed installation is a must on kernel build machines), also executing a pre build script as a part of vc project did not look straightforward when i tried it last time (for some other issue). Why we picked approach 1: a. It alligns with the odp-netlink.h generation. b. It will unblock the windows kernel build easily. c. After committing this patch i'll investigate approach 2 and implement that if everyone want to. Please let me know your thoughts, we can approach the issue accordingly. Regards, Ankur From: Alin Serdean Sent: Wednesday, August 6, 2014 6:17 PM To: Nithin Raju; Ankur Sharma Cc: Subject: RE: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a versionof odp-netlink for windows kernel. I was wondering why not generate the #ifdef over the file that already generates a copy of openvswitch.h (odp-netlink.h) needed for the extension? And as Nithin pointed out I think this can be solved in a more elegant solution. Thanks, Alin. -Mesaj original- De la: dev [mailto:dev-boun...@openvswitch.org] În numele Nithin Raju Trimis: Thursday, August 7, 2014 3:17 AM Către: Ankur Sharma Cc: Subiect: Re: [ovs-dev] [PATCH]
[ovs-dev] [PATCH] datapath: Optimize recirc action.
OVS need to flow key for flow lookup in recic action. OVS
does key extract in recic action. Most of cases we could
use OVS_CB packet key directly and can avoid packet flow key
extract. SET action we can update flow-key along with packet
to keep it consistent. But there are some action like MPLS
pop which forces OVS to do flow-extract. In such cases we
can mark flow key as invalid so that subsequent recirc
action can do full flow extract.
Signed-off-by: Pravin B Shelar
---
datapath/actions.c | 197 +++--
1 file changed, 160 insertions(+), 37 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index efc64f1..3c004a8 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -40,6 +40,90 @@
#include "vlan.h"
#include "vport.h"
+static void flow_key_set_priority(struct sk_buff *skb, u32 priority)
+{
+ OVS_CB(skb)->pkt_key->phy.priority = priority;
+}
+
+static void flow_key_set_skb_mark(struct sk_buff *skb, u32 skb_mark)
+{
+ OVS_CB(skb)->pkt_key->phy.skb_mark = skb_mark;
+}
+
+static void flow_key_set_eth_src(struct sk_buff *skb, const u8 addr[])
+{
+ ether_addr_copy(OVS_CB(skb)->pkt_key->eth.src, addr);
+}
+
+static void flow_key_set_eth_dst(struct sk_buff *skb, const u8 addr[])
+{
+ ether_addr_copy(OVS_CB(skb)->pkt_key->eth.dst, addr);
+}
+
+static void flow_key_set_vlan_tci(struct sk_buff *skb, __be16 tci)
+{
+ OVS_CB(skb)->pkt_key->eth.tci = tci;
+}
+
+static void flow_key_set_ipv4_src(struct sk_buff *skb, __be32 addr)
+{
+ OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
+}
+
+static void flow_key_set_ipv4_dst(struct sk_buff *skb, __be32 addr)
+{
+ OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
+}
+
+static void flow_key_set_ipv4_tos(struct sk_buff *skb, const struct iphdr *nh)
+{
+ OVS_CB(skb)->pkt_key->ip.tos = nh->tos;
+}
+
+static void flow_key_set_ip_ttl(struct sk_buff *skb, u8 ttl)
+{
+ OVS_CB(skb)->pkt_key->ip.ttl = ttl;
+}
+
+static void flow_key_set_ipv6_src(struct sk_buff *skb,
+ const __be32 addr[4])
+{
+ memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.src, addr, sizeof(__be32[4]));
+}
+
+static void flow_key_set_ipv6_dst(struct sk_buff *skb,
+ const __be32 addr[4])
+{
+ memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.dst, addr, sizeof(__be32[4]));
+}
+
+static void flow_key_set_ipv6_fl(struct sk_buff *skb,
+const struct ipv6hdr *nh)
+{
+ OVS_CB(skb)->pkt_key->ipv6.label = *(__be32 *)nh &
+ htonl(IPV6_FLOWINFO_FLOWLABEL);
+}
+
+static void flow_key_set_tp_src(struct sk_buff *skb, __be16 port)
+{
+ OVS_CB(skb)->pkt_key->tp.src = port;
+}
+
+static void flow_key_set_tp_dst(struct sk_buff *skb, __be16 port)
+{
+ OVS_CB(skb)->pkt_key->tp.dst = port;
+}
+
+static void invalidate_skb_flow_key(struct sk_buff *skb)
+{
+ OVS_CB(skb)->pkt_key->eth.type = 0;
+}
+
+static bool is_skb_flow_key_valid(struct sk_buff *skb)
+{
+ return !!OVS_CB(skb)->pkt_key->eth.type;
+}
+
static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
const struct nlattr *attr, int len);
@@ -90,6 +174,7 @@ static int push_mpls(struct sk_buff *skb,
if (!ovs_skb_get_inner_protocol(skb))
ovs_skb_set_inner_protocol(skb, skb->protocol);
skb->protocol = mpls->mpls_ethertype;
+ invalidate_skb_flow_key(skb);
return 0;
}
@@ -120,6 +205,7 @@ static int pop_mpls(struct sk_buff *skb, const __be16
ethertype)
hdr->h_proto = ethertype;
if (eth_p_mpls(skb->protocol))
skb->protocol = ethertype;
+ invalidate_skb_flow_key(skb);
return 0;
}
@@ -139,7 +225,7 @@ static int set_mpls(struct sk_buff *skb, const __be32
*mpls_lse)
}
*stack = *mpls_lse;
-
+ invalidate_skb_flow_key(skb);
return 0;
}
@@ -189,9 +275,12 @@ static int pop_vlan(struct sk_buff *skb)
}
/* move next vlan tag to hw accel tag */
if (likely(skb->protocol != htons(ETH_P_8021Q) ||
- skb->len < VLAN_ETH_HLEN))
+ skb->len < VLAN_ETH_HLEN)) {
+ flow_key_set_vlan_tci(skb, 0);
return 0;
+ }
+ invalidate_skb_flow_key(skb);
err = __pop_vlan_tci(skb, &tci);
if (unlikely(err))
return err;
@@ -220,6 +309,7 @@ static int push_vlan(struct sk_buff *skb, const struct
ovs_action_push_vlan *vla
}
__vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) &
~VLAN_TAG_PRESENT);
+ invalidate_skb_flow_key(skb);
return 0;
}
@@ -238,11 +328,13 @@ static int set_eth_addr(struct sk_buff *skb,
ovs_skb_postpush_rcsum(skb, eth_hdr(skb), ETH_ALEN * 2);
+ flow_key_set_eth_src(skb, eth_key->eth_src);
+ flow_key_set_eth_dst(skb, eth_key->eth_dst);
return 0;
[ovs-dev] [PATCH] dpdk: Update documentation.
Add usage info for ovs-vswitchd dpdk option. Update
INSTALL.DPDK file.
Reported-by: Hari Sasank Bhamidipalli
Signed-off-by: Pravin B Shelar
---
AUTHORS | 1 +
INSTALL.DPDK| 5 ++---
vswitchd/ovs-vswitchd.c | 2 ++
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index d1756ee..8c5940d 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -201,6 +201,7 @@ Greg Dahlmangdahl...@hotmail.com
Gregor Schaffrath gr...@net.t-labs.tu-berlin.de
Guolin Yang gy...@vmware.com
Gur Stavi gst...@mrv.com
+Hari Sasank Bhamidipalli hbham...@cisco.com
Hassan Khan hassan.k...@seecs.edu.pk
Hector Oron hector.o...@gmail.com
Henrik Amrenhen...@nicira.com
diff --git a/INSTALL.DPDK b/INSTALL.DPDK
index c74fa5c..4551f4c 100644
--- a/INSTALL.DPDK
+++ b/INSTALL.DPDK
@@ -72,8 +72,6 @@ First setup DPDK devices:
:02:00.0 :02:00.1 bind module new_id remove_id uevent
unbind
Prepare system:
- - load ovs kernel module
-e.g modprobe openvswitch
- mount hugetlbfs
e.g. mount -t hugetlbfs -o pagesize=1G none /mnt/huge/
@@ -101,7 +99,8 @@ Start ovsdb-server as discussed in INSTALL doc:
Start vswitchd:
DPDK configuration arguments can be passed to vswitchd via `--dpdk`
-argument. dpdk arg -c is ignored by ovs-dpdk, but it is a required parameter
+argument. This needs to be first argument passed to vswitchd process.
+dpdk arg -c is ignored by ovs-dpdk, but it is a required parameter
for dpdk initialization.
e.g.
diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
index 3c843e1..199db2d 100644
--- a/vswitchd/ovs-vswitchd.c
+++ b/vswitchd/ovs-vswitchd.c
@@ -250,6 +250,8 @@ usage(void)
stream_usage("DATABASE", true, false, true);
daemon_usage();
vlog_usage();
+printf("\nDPDK options:\n"
+ " --dpdk options See INSTALL.DPDK for more info.\n");
printf("\nOther options:\n"
" --unixctl=SOCKEToverride default control socket name\n"
" -h, --help display this help message\n"
--
1.9.3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath: Refactor action alloc and copy api.
There are two separate API to allocate and copy actions list. Anytime
OVS needs to copy action list, it needs to call both functions.
Following patch moves action allocation to copy function to avoid
code duplication.
Signed-off-by: Pravin B Shelar
---
datapath/datapath.c | 49 +
datapath/flow_netlink.c | 24 +---
datapath/flow_netlink.h | 1 -
3 files changed, 26 insertions(+), 48 deletions(-)
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 19d41c8..62e3c26 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -569,17 +569,12 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
struct genl_info *info)
if (err)
goto err_flow_free;
- acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_PACKET_ATTR_ACTIONS]));
- err = PTR_ERR(acts);
- if (IS_ERR(acts))
- goto err_flow_free;
-
err = ovs_nla_copy_actions(a[OVS_PACKET_ATTR_ACTIONS],
&flow->key, &acts);
- rcu_assign_pointer(flow->sf_acts, acts);
if (err)
goto err_flow_free;
+ rcu_assign_pointer(flow->sf_acts, acts);
OVS_CB(packet)->flow = flow;
OVS_CB(packet)->pkt_key = &flow->key;
OVS_CB(skb)->egress_tun_info = NULL;
@@ -899,11 +894,6 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
genl_info *info)
ovs_flow_mask_key(&new_flow->key, &new_flow->unmasked_key, &mask);
/* Validate actions. */
- acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_FLOW_ATTR_ACTIONS]));
- error = PTR_ERR(acts);
- if (IS_ERR(acts))
- goto err_kfree_flow;
-
error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &new_flow->key,
&acts);
if (error) {
@@ -1000,29 +990,6 @@ error:
return error;
}
-static struct sw_flow_actions *get_flow_actions(const struct nlattr *a,
- const struct sw_flow_key *key,
- const struct sw_flow_mask *mask)
-{
- struct sw_flow_actions *acts;
- struct sw_flow_key masked_key;
- int error;
-
- acts = ovs_nla_alloc_flow_actions(nla_len(a));
- if (IS_ERR(acts))
- return acts;
-
- ovs_flow_mask_key(&masked_key, key, mask);
- error = ovs_nla_copy_actions(a, &masked_key, &acts);
- if (error) {
- OVS_NLERR("Flow actions may not be safe on all matching
packets.\n");
- kfree(acts);
- return ERR_PTR(error);
- }
-
- return acts;
-}
-
static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
{
struct nlattr **a = info->attrs;
@@ -1051,15 +1018,17 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct
genl_info *info)
/* Validate actions. */
if (a[OVS_FLOW_ATTR_ACTIONS]) {
- acts = get_flow_actions(a[OVS_FLOW_ATTR_ACTIONS], &key, &mask);
- if (IS_ERR(acts)) {
- error = PTR_ERR(acts);
+ struct sw_flow_key masked_key;
+
+ ovs_flow_mask_key(&masked_key, &key, &mask);
+ error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
+&masked_key, &acts);
+ if (error) {
+ OVS_NLERR("Flow actions may not be safe on all matching
packets.\n");
goto error;
}
- }
- /* Can allocate before locking if have acts. */
- if (acts) {
+ /* Can allocate before locking if have acts. */
reply = ovs_flow_cmd_alloc_info(acts, info, false);
if (IS_ERR(reply)) {
error = PTR_ERR(reply);
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 75172de..e4cf535 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -1239,7 +1239,7 @@ nla_put_failure:
#define MAX_ACTIONS_BUFSIZE(32 * 1024)
-struct sw_flow_actions *ovs_nla_alloc_flow_actions(int size)
+static struct sw_flow_actions *nla_alloc_flow_actions(int size)
{
struct sw_flow_actions *sfa;
@@ -1293,7 +1293,7 @@ static struct nlattr *reserve_sfa_size(struct
sw_flow_actions **sfa,
new_acts_size = MAX_ACTIONS_BUFSIZE;
}
- acts = ovs_nla_alloc_flow_actions(new_acts_size);
+ acts = nla_alloc_flow_actions(new_acts_size);
if (IS_ERR(acts))
return (void *)acts;
@@ -1360,7 +1360,7 @@ static inline void add_nested_action_end(struct
sw_flow_actions *sfa,
a->nla_len = sfa->actions_len - st_offset;
}
-static int ovs_nla_copy_actions__(const struct nlattr *attr,
+static int __ovs_nla_copy_actions(const struct nlattr *attr,
const struct sw_flow_key *key,
int depth
Re: [ovs-dev] [PATCH] dpdk: Update documentation.
On Thu, Aug 07, 2014 at 12:40:34PM -0700, Pravin B Shelar wrote: > Add usage info for ovs-vswitchd dpdk option. Update > INSTALL.DPDK file. > > Reported-by: Hari Sasank Bhamidipalli > Signed-off-by: Pravin B Shelar I see that this references INSTALL.DPDK from the --help message. It looks like there's only a few sentences in INSTALL.DPDK on the --dpdk option. To me, that makes it seem like it would be easy enough to add those sentences to ovs-vswitchd(8), and then people would easily find the documentation in the expected place. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v7] Extend OVS IPFIX exporter to export tunnel headers
On Thu, Aug 07, 2014 at 06:20:41AM +, Wenyu Zhang wrote: > > > I'm not sure why tnl_xlate_init() has this new code: > > > /* The tp_src and tp_dst members in flow_tnl are set to be always > > > * wildcarded, not to unwildcard them here. */ > > > wc->masks.tunnel.tp_src = 0; > > > wc->masks.tunnel.tp_dst = 0; Does it have any visible > > > effect? It appears to me that these fields are already 0, as initialized > > > by the caller. > > Yes, the caller has initialized the whole data strusture in the > > existent codes. But we think that we should set the two fileds as 0 > > obviously in this function, to avoid potential issue if it is called > > by other callers in future. > > OK, can you please submit that as a separate patch since it is a logically > separate change? > > Wenyu: The tp_src and tp_dst is new added fields in this patch. I am afraid > that it is not a good idea to separate it in another patch. I missed that. Thanks, please keep this change in this patch then. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Optimize recirc action.
Most of the time we do not need the updated key (i.e., when there is no recirc
action). Do you think it would be worthwhile to only update the key when
needed, e.g. by adding a “bool update_key;” member to struct sw_flow_actions?
More comments below,
Jarno
On Aug 7, 2014, at 12:32 PM, Pravin B Shelar wrote:
> OVS need to flow key for flow lookup in recic action. OVS
> does key extract in recic action. Most of cases we could
> use OVS_CB packet key directly and can avoid packet flow key
> extract. SET action we can update flow-key along with packet
> to keep it consistent. But there are some action like MPLS
> pop which forces OVS to do flow-extract. In such cases we
> can mark flow key as invalid so that subsequent recirc
> action can do full flow extract.
>
> Signed-off-by: Pravin B Shelar
> ---
> datapath/actions.c | 197 +++--
> 1 file changed, 160 insertions(+), 37 deletions(-)
>
> diff --git a/datapath/actions.c b/datapath/actions.c
> index efc64f1..3c004a8 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -40,6 +40,90 @@
> #include "vlan.h"
> #include "vport.h"
>
> +static void flow_key_set_priority(struct sk_buff *skb, u32 priority)
> +{
> + OVS_CB(skb)->pkt_key->phy.priority = priority;
> +}
> +
> +static void flow_key_set_skb_mark(struct sk_buff *skb, u32 skb_mark)
> +{
> + OVS_CB(skb)->pkt_key->phy.skb_mark = skb_mark;
> +}
> +
> +static void flow_key_set_eth_src(struct sk_buff *skb, const u8 addr[])
> +{
> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.src, addr);
> +}
> +
> +static void flow_key_set_eth_dst(struct sk_buff *skb, const u8 addr[])
> +{
> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.dst, addr);
> +}
> +
> +static void flow_key_set_vlan_tci(struct sk_buff *skb, __be16 tci)
> +{
> + OVS_CB(skb)->pkt_key->eth.tci = tci;
> +}
> +
> +static void flow_key_set_ipv4_src(struct sk_buff *skb, __be32 addr)
> +{
> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
> +}
> +
> +static void flow_key_set_ipv4_dst(struct sk_buff *skb, __be32 addr)
> +{
> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
> +}
> +
> +static void flow_key_set_ipv4_tos(struct sk_buff *skb, const struct iphdr
> *nh)
> +{
> + OVS_CB(skb)->pkt_key->ip.tos = nh->tos;
> +}
> +
> +static void flow_key_set_ip_ttl(struct sk_buff *skb, u8 ttl)
> +{
> + OVS_CB(skb)->pkt_key->ip.ttl = ttl;
> +}
> +
> +static void flow_key_set_ipv6_src(struct sk_buff *skb,
> + const __be32 addr[4])
> +{
> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.src, addr, sizeof(__be32[4]));
> +}
> +
> +static void flow_key_set_ipv6_dst(struct sk_buff *skb,
> + const __be32 addr[4])
> +{
> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.dst, addr, sizeof(__be32[4]));
> +}
> +
> +static void flow_key_set_ipv6_fl(struct sk_buff *skb,
> + const struct ipv6hdr *nh)
> +{
> + OVS_CB(skb)->pkt_key->ipv6.label = *(__be32 *)nh &
> +htonl(IPV6_FLOWINFO_FLOWLABEL);
> +}
> +
> +static void flow_key_set_tp_src(struct sk_buff *skb, __be16 port)
> +{
> + OVS_CB(skb)->pkt_key->tp.src = port;
> +}
> +
> +static void flow_key_set_tp_dst(struct sk_buff *skb, __be16 port)
> +{
> + OVS_CB(skb)->pkt_key->tp.dst = port;
> +}
> +
> +static void invalidate_skb_flow_key(struct sk_buff *skb)
> +{
> + OVS_CB(skb)->pkt_key->eth.type = 0;
> +}
> +
> +static bool is_skb_flow_key_valid(struct sk_buff *skb)
> +{
> + return !!OVS_CB(skb)->pkt_key->eth.type;
> +}
> +
> static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> const struct nlattr *attr, int len);
>
> @@ -90,6 +174,7 @@ static int push_mpls(struct sk_buff *skb,
> if (!ovs_skb_get_inner_protocol(skb))
> ovs_skb_set_inner_protocol(skb, skb->protocol);
> skb->protocol = mpls->mpls_ethertype;
> + invalidate_skb_flow_key(skb);
> return 0;
> }
>
> @@ -120,6 +205,7 @@ static int pop_mpls(struct sk_buff *skb, const __be16
> ethertype)
> hdr->h_proto = ethertype;
> if (eth_p_mpls(skb->protocol))
> skb->protocol = ethertype;
> + invalidate_skb_flow_key(skb);
> return 0;
> }
>
> @@ -139,7 +225,7 @@ static int set_mpls(struct sk_buff *skb, const __be32
> *mpls_lse)
> }
>
> *stack = *mpls_lse;
> -
> + invalidate_skb_flow_key(skb);
Updating the OVS_CB(skb)->pkt_key->mpls.top_lse should be done instead of
invalidating the flow key.
> return 0;
> }
>
> @@ -189,9 +275,12 @@ static int pop_vlan(struct sk_buff *skb)
> }
> /* move next vlan tag to hw accel tag */
> if (likely(skb->protocol != htons(ETH_P_8021Q) ||
> -skb->len < VLAN_ETH_HLEN))
> +skb->len < VLAN_ETH_HLEN)) {
> + flow_key_set_vlan_tci(skb, 0);
> return 0;
> + }
>
> + invalidate_skb_flow_key(skb);
> err =
Re: [ovs-dev] [PATCH] dpdk: Update documentation.
On Thu, Aug 7, 2014 at 12:55 PM, Ben Pfaff wrote: > On Thu, Aug 07, 2014 at 12:40:34PM -0700, Pravin B Shelar wrote: >> Add usage info for ovs-vswitchd dpdk option. Update >> INSTALL.DPDK file. >> >> Reported-by: Hari Sasank Bhamidipalli >> Signed-off-by: Pravin B Shelar > > I see that this references INSTALL.DPDK from the --help message. It > looks like there's only a few sentences in INSTALL.DPDK on the --dpdk > option. To me, that makes it seem like it would be easy enough to add > those sentences to ovs-vswitchd(8), and then people would easily find > the documentation in the expected place. I thought abut that. But just having --dpdk option does not work. User will have to refer INSTALL.DPDK doc for other setup steps. So rather than reading two docs, user can have all information about dpdk setup in INSTALL.DPDK. I can add a pointer to INSTALL.DPDK in man page, what do you think? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpdk: Update documentation.
On Thu, Aug 07, 2014 at 01:46:52PM -0700, Pravin Shelar wrote: > On Thu, Aug 7, 2014 at 12:55 PM, Ben Pfaff wrote: > > On Thu, Aug 07, 2014 at 12:40:34PM -0700, Pravin B Shelar wrote: > >> Add usage info for ovs-vswitchd dpdk option. Update > >> INSTALL.DPDK file. > >> > >> Reported-by: Hari Sasank Bhamidipalli > >> Signed-off-by: Pravin B Shelar > > > > I see that this references INSTALL.DPDK from the --help message. It > > looks like there's only a few sentences in INSTALL.DPDK on the --dpdk > > option. To me, that makes it seem like it would be easy enough to add > > those sentences to ovs-vswitchd(8), and then people would easily find > > the documentation in the expected place. > > I thought abut that. > But just having --dpdk option does not work. User will have to refer > INSTALL.DPDK doc for other setup steps. So rather than reading two > docs, user can have all information about dpdk setup in INSTALL.DPDK. > > I can add a pointer to INSTALL.DPDK in man page, what do you think? That will also help users find the documentation, so I think that's a good approach for now. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Optimize recirc action.
On Thu, Aug 7, 2014 at 12:32 PM, Pravin B Shelar wrote:
> OVS need to flow key for flow lookup in recic action. OVS
> does key extract in recic action. Most of cases we could
> use OVS_CB packet key directly and can avoid packet flow key
> extract. SET action we can update flow-key along with packet
> to keep it consistent. But there are some action like MPLS
> pop which forces OVS to do flow-extract. In such cases we
> can mark flow key as invalid so that subsequent recirc
> action can do full flow extract.
Does this optimization apply to sample action as well?
>
> Signed-off-by: Pravin B Shelar
> ---
> datapath/actions.c | 197
> +++--
> 1 file changed, 160 insertions(+), 37 deletions(-)
>
> diff --git a/datapath/actions.c b/datapath/actions.c
> index efc64f1..3c004a8 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -40,6 +40,90 @@
> #include "vlan.h"
> #include "vport.h"
>
> +static void flow_key_set_priority(struct sk_buff *skb, u32 priority)
Why not just pass in pkt_key, instead of skb?
> +{
> + OVS_CB(skb)->pkt_key->phy.priority = priority;
> +}
> +
> +static void flow_key_set_skb_mark(struct sk_buff *skb, u32 skb_mark)
> +{
> + OVS_CB(skb)->pkt_key->phy.skb_mark = skb_mark;
> +}
> +
> +static void flow_key_set_eth_src(struct sk_buff *skb, const u8 addr[])
> +{
> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.src, addr);
> +}
> +
> +static void flow_key_set_eth_dst(struct sk_buff *skb, const u8 addr[])
> +{
> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.dst, addr);
> +}
> +
> +static void flow_key_set_vlan_tci(struct sk_buff *skb, __be16 tci)
> +{
> + OVS_CB(skb)->pkt_key->eth.tci = tci;
> +}
> +
> +static void flow_key_set_ipv4_src(struct sk_buff *skb, __be32 addr)
> +{
> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
> +}
> +
> +static void flow_key_set_ipv4_dst(struct sk_buff *skb, __be32 addr)
> +{
> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
> +}
> +
> +static void flow_key_set_ipv4_tos(struct sk_buff *skb, const struct iphdr
> *nh)
> +{
> + OVS_CB(skb)->pkt_key->ip.tos = nh->tos;
> +}
> +
> +static void flow_key_set_ip_ttl(struct sk_buff *skb, u8 ttl)
> +{
> + OVS_CB(skb)->pkt_key->ip.ttl = ttl;
> +}
> +
> +static void flow_key_set_ipv6_src(struct sk_buff *skb,
> + const __be32 addr[4])
> +{
> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.src, addr, sizeof(__be32[4]));
> +}
> +
> +static void flow_key_set_ipv6_dst(struct sk_buff *skb,
> + const __be32 addr[4])
> +{
> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.dst, addr, sizeof(__be32[4]));
> +}
> +
> +static void flow_key_set_ipv6_fl(struct sk_buff *skb,
> +const struct ipv6hdr *nh)
> +{
> + OVS_CB(skb)->pkt_key->ipv6.label = *(__be32 *)nh &
> + htonl(IPV6_FLOWINFO_FLOWLABEL);
> +}
> +
> +static void flow_key_set_tp_src(struct sk_buff *skb, __be16 port)
> +{
> + OVS_CB(skb)->pkt_key->tp.src = port;
> +}
> +
> +static void flow_key_set_tp_dst(struct sk_buff *skb, __be16 port)
> +{
> + OVS_CB(skb)->pkt_key->tp.dst = port;
> +}
> +
> +static void invalidate_skb_flow_key(struct sk_buff *skb)
> +{
> + OVS_CB(skb)->pkt_key->eth.type = 0;
> +}
> +
> +static bool is_skb_flow_key_valid(struct sk_buff *skb)
> +{
> + return !!OVS_CB(skb)->pkt_key->eth.type;
> +}
> +
> static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> const struct nlattr *attr, int len);
>
> @@ -90,6 +174,7 @@ static int push_mpls(struct sk_buff *skb,
> if (!ovs_skb_get_inner_protocol(skb))
> ovs_skb_set_inner_protocol(skb, skb->protocol);
> skb->protocol = mpls->mpls_ethertype;
> + invalidate_skb_flow_key(skb);
> return 0;
> }
>
> @@ -120,6 +205,7 @@ static int pop_mpls(struct sk_buff *skb, const __be16
> ethertype)
> hdr->h_proto = ethertype;
> if (eth_p_mpls(skb->protocol))
> skb->protocol = ethertype;
> + invalidate_skb_flow_key(skb);
> return 0;
> }
>
> @@ -139,7 +225,7 @@ static int set_mpls(struct sk_buff *skb, const __be32
> *mpls_lse)
> }
>
> *stack = *mpls_lse;
> -
> + invalidate_skb_flow_key(skb);
> return 0;
> }
>
> @@ -189,9 +275,12 @@ static int pop_vlan(struct sk_buff *skb)
> }
> /* move next vlan tag to hw accel tag */
> if (likely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN))
> + skb->len < VLAN_ETH_HLEN)) {
> + flow_key_set_vlan_tci(skb, 0);
> return 0;
> + }
>
> + invalidate_skb_flow_key(skb);
> err = __pop_vlan_tci(skb, &tci);
> if (unlikely(err))
> return err;
> @@ -220,6 +309,7 @@ static int push_vlan(struct sk_buff *skb, const struct
> ov
Re: [ovs-dev] [PATCH] datapath: Optimize recirc action.
On Thu, Aug 7, 2014 at 1:27 PM, Jarno Rajahalme wrote:
> Most of the time we do not need the updated key (i.e., when there is no
> recirc action). Do you think it would be worthwhile to only update the key
> when needed, e.g. by adding a “bool update_key;” member to struct
> sw_flow_actions?
>
It is good idea. I am not sure about performance benefit we will see
this. We can look at it later if we find issue with current approach.
> More comments below,
>
> Jarno
>
> On Aug 7, 2014, at 12:32 PM, Pravin B Shelar wrote:
>
>> OVS need to flow key for flow lookup in recic action. OVS
>> does key extract in recic action. Most of cases we could
>> use OVS_CB packet key directly and can avoid packet flow key
>> extract. SET action we can update flow-key along with packet
>> to keep it consistent. But there are some action like MPLS
>> pop which forces OVS to do flow-extract. In such cases we
>> can mark flow key as invalid so that subsequent recirc
>> action can do full flow extract.
>>
>> Signed-off-by: Pravin B Shelar
>> ---
>> datapath/actions.c | 197
>> +++--
>> 1 file changed, 160 insertions(+), 37 deletions(-)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index efc64f1..3c004a8 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -40,6 +40,90 @@
>> #include "vlan.h"
>> #include "vport.h"
>>
>> +static void flow_key_set_priority(struct sk_buff *skb, u32 priority)
>> +{
>> + OVS_CB(skb)->pkt_key->phy.priority = priority;
>> +}
>> +
>> +static void flow_key_set_skb_mark(struct sk_buff *skb, u32 skb_mark)
>> +{
>> + OVS_CB(skb)->pkt_key->phy.skb_mark = skb_mark;
>> +}
>> +
>> +static void flow_key_set_eth_src(struct sk_buff *skb, const u8 addr[])
>> +{
>> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.src, addr);
>> +}
>> +
>> +static void flow_key_set_eth_dst(struct sk_buff *skb, const u8 addr[])
>> +{
>> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.dst, addr);
>> +}
>> +
>> +static void flow_key_set_vlan_tci(struct sk_buff *skb, __be16 tci)
>> +{
>> + OVS_CB(skb)->pkt_key->eth.tci = tci;
>> +}
>> +
>> +static void flow_key_set_ipv4_src(struct sk_buff *skb, __be32 addr)
>> +{
>> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
>> +}
>> +
>> +static void flow_key_set_ipv4_dst(struct sk_buff *skb, __be32 addr)
>> +{
>> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
>> +}
>> +
>> +static void flow_key_set_ipv4_tos(struct sk_buff *skb, const struct iphdr
>> *nh)
>> +{
>> + OVS_CB(skb)->pkt_key->ip.tos = nh->tos;
>> +}
>> +
>> +static void flow_key_set_ip_ttl(struct sk_buff *skb, u8 ttl)
>> +{
>> + OVS_CB(skb)->pkt_key->ip.ttl = ttl;
>> +}
>> +
>> +static void flow_key_set_ipv6_src(struct sk_buff *skb,
>> + const __be32 addr[4])
>> +{
>> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.src, addr, sizeof(__be32[4]));
>> +}
>> +
>> +static void flow_key_set_ipv6_dst(struct sk_buff *skb,
>> + const __be32 addr[4])
>> +{
>> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.dst, addr, sizeof(__be32[4]));
>> +}
>> +
>> +static void flow_key_set_ipv6_fl(struct sk_buff *skb,
>> + const struct ipv6hdr *nh)
>> +{
>> + OVS_CB(skb)->pkt_key->ipv6.label = *(__be32 *)nh &
>> +htonl(IPV6_FLOWINFO_FLOWLABEL);
>> +}
>> +
>> +static void flow_key_set_tp_src(struct sk_buff *skb, __be16 port)
>> +{
>> + OVS_CB(skb)->pkt_key->tp.src = port;
>> +}
>> +
>> +static void flow_key_set_tp_dst(struct sk_buff *skb, __be16 port)
>> +{
>> + OVS_CB(skb)->pkt_key->tp.dst = port;
>> +}
>> +
>> +static void invalidate_skb_flow_key(struct sk_buff *skb)
>> +{
>> + OVS_CB(skb)->pkt_key->eth.type = 0;
>> +}
>> +
>> +static bool is_skb_flow_key_valid(struct sk_buff *skb)
>> +{
>> + return !!OVS_CB(skb)->pkt_key->eth.type;
>> +}
>> +
>> static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>> const struct nlattr *attr, int len);
>>
>> @@ -90,6 +174,7 @@ static int push_mpls(struct sk_buff *skb,
>> if (!ovs_skb_get_inner_protocol(skb))
>> ovs_skb_set_inner_protocol(skb, skb->protocol);
>> skb->protocol = mpls->mpls_ethertype;
>> + invalidate_skb_flow_key(skb);
>> return 0;
>> }
>>
>> @@ -120,6 +205,7 @@ static int pop_mpls(struct sk_buff *skb, const __be16
>> ethertype)
>> hdr->h_proto = ethertype;
>> if (eth_p_mpls(skb->protocol))
>> skb->protocol = ethertype;
>> + invalidate_skb_flow_key(skb);
>> return 0;
>> }
>>
>> @@ -139,7 +225,7 @@ static int set_mpls(struct sk_buff *skb, const __be32
>> *mpls_lse)
>> }
>>
>> *stack = *mpls_lse;
>> -
>> + invalidate_skb_flow_key(skb);
>
> Updating the OVS_CB(skb)->pkt_key->mpls.top_lse should be done instead of
> invalidating the flow key.
>
ok.
>> return 0;
>> }
>>
>> @@ -189,9 +275,12 @@ static int pop_vlan(struct sk_
Re: [ovs-dev] [PATCH 1/2] datapath: Update comments about 'OVS_KEY_ATTR_8021Q'.
On August 7, 2014 at 10:09:17 AM, Pravin Shelar (pshe...@nicira.com) wrote: > On Wed, Aug 6, 2014 at 2:55 PM, Justin Pettit wrote: > > Commit fea393b1 (datapath: Describe policy for extending flow key, > > implement needed changes.) changed the key 'OVS_KEY_ATTR_8021Q' to > > 'OVS_KEY_ATTR_VLAN' and the size of the attribute structure. A couple > > of comments were missed, so this commit updates them. > > > > Signed-off-by: Justin Pettit > > --- > > datapath/datapath.c | 2 +- > > lib/odp-util.h | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/datapath/datapath.c b/datapath/datapath.c > > index 1185f60..26d060e 100644 > > --- a/datapath/datapath.c > > +++ b/datapath/datapath.c > > @@ -409,7 +409,7 @@ static size_t key_attr_size(void) > > + nla_total_size(4) /* OVS_KEY_ATTR_RECIRC_ID */ > > + nla_total_size(12) /* OVS_KEY_ATTR_ETHERNET */ > > + nla_total_size(2) /* OVS_KEY_ATTR_ETHERTYPE */ > > - + nla_total_size(4) /* OVS_KEY_ATTR_8021Q */ > > + + nla_total_size(4) /* OVS_KEY_ATTR_VLAN */ > > + nla_total_size(0) /* OVS_KEY_ATTR_ENCAP */ > > + nla_total_size(2) /* OVS_KEY_ATTR_ETHERTYPE */ > > + nla_total_size(40) /* OVS_KEY_ATTR_IPV6 */ > > diff --git a/lib/odp-util.h b/lib/odp-util.h > > index 82ab06d..a0c0ae8 100644 > > --- a/lib/odp-util.h > > +++ b/lib/odp-util.h > > @@ -112,7 +112,7 @@ void odp_portno_names_destroy(struct hmap > > *portno_names); > > * OVS_KEY_ATTR_RECIRC_ID 4 -- 4 8 > > * OVS_KEY_ATTR_ETHERNET 12 -- 4 16 > > * OVS_KEY_ATTR_ETHERTYPE 2 2 4 8 (outer VLAN ethertype) > > - * OVS_KEY_ATTR_8021Q 4 -- 4 8 > > + * OVS_KEY_ATTR_VLAN 2 2 4 8 > > * OVS_KEY_ATTR_ENCAP 0 -- 4 4 (VLAN encapsulation) > > * OVS_KEY_ATTR_ETHERTYPE 2 2 4 8 (inner VLAN ethertype) > > * OVS_KEY_ATTR_IPV6 40 -- 4 44 > > -- > > LGTM. > > Acked-by: Pravin B Shelar Thanks. I pushed it. --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Refactor action alloc and copy api.
Acked-by: Jarno Rajahalme
On Aug 7, 2014, at 12:51 PM, Pravin B Shelar wrote:
> There are two separate API to allocate and copy actions list. Anytime
> OVS needs to copy action list, it needs to call both functions.
> Following patch moves action allocation to copy function to avoid
> code duplication.
>
> Signed-off-by: Pravin B Shelar
> ---
> datapath/datapath.c | 49 +
> datapath/flow_netlink.c | 24 +---
> datapath/flow_netlink.h | 1 -
> 3 files changed, 26 insertions(+), 48 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 19d41c8..62e3c26 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -569,17 +569,12 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
> struct genl_info *info)
> if (err)
> goto err_flow_free;
>
> - acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_PACKET_ATTR_ACTIONS]));
> - err = PTR_ERR(acts);
> - if (IS_ERR(acts))
> - goto err_flow_free;
> -
> err = ovs_nla_copy_actions(a[OVS_PACKET_ATTR_ACTIONS],
> &flow->key, &acts);
> - rcu_assign_pointer(flow->sf_acts, acts);
> if (err)
> goto err_flow_free;
>
> + rcu_assign_pointer(flow->sf_acts, acts);
> OVS_CB(packet)->flow = flow;
> OVS_CB(packet)->pkt_key = &flow->key;
> OVS_CB(skb)->egress_tun_info = NULL;
> @@ -899,11 +894,6 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
> ovs_flow_mask_key(&new_flow->key, &new_flow->unmasked_key, &mask);
>
> /* Validate actions. */
> - acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_FLOW_ATTR_ACTIONS]));
> - error = PTR_ERR(acts);
> - if (IS_ERR(acts))
> - goto err_kfree_flow;
> -
> error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &new_flow->key,
>&acts);
> if (error) {
> @@ -1000,29 +990,6 @@ error:
> return error;
> }
>
> -static struct sw_flow_actions *get_flow_actions(const struct nlattr *a,
> - const struct sw_flow_key *key,
> - const struct sw_flow_mask *mask)
> -{
> - struct sw_flow_actions *acts;
> - struct sw_flow_key masked_key;
> - int error;
> -
> - acts = ovs_nla_alloc_flow_actions(nla_len(a));
> - if (IS_ERR(acts))
> - return acts;
> -
> - ovs_flow_mask_key(&masked_key, key, mask);
> - error = ovs_nla_copy_actions(a, &masked_key, &acts);
> - if (error) {
> - OVS_NLERR("Flow actions may not be safe on all matching
> packets.\n");
> - kfree(acts);
> - return ERR_PTR(error);
> - }
> -
> - return acts;
> -}
> -
> static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> {
> struct nlattr **a = info->attrs;
> @@ -1051,15 +1018,17 @@ static int ovs_flow_cmd_set(struct sk_buff *skb,
> struct genl_info *info)
>
> /* Validate actions. */
> if (a[OVS_FLOW_ATTR_ACTIONS]) {
> - acts = get_flow_actions(a[OVS_FLOW_ATTR_ACTIONS], &key, &mask);
> - if (IS_ERR(acts)) {
> - error = PTR_ERR(acts);
> + struct sw_flow_key masked_key;
> +
> + ovs_flow_mask_key(&masked_key, &key, &mask);
> + error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
> + &masked_key, &acts);
> + if (error) {
> + OVS_NLERR("Flow actions may not be safe on all matching
> packets.\n");
> goto error;
> }
> - }
>
> - /* Can allocate before locking if have acts. */
> - if (acts) {
> + /* Can allocate before locking if have acts. */
> reply = ovs_flow_cmd_alloc_info(acts, info, false);
> if (IS_ERR(reply)) {
> error = PTR_ERR(reply);
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 75172de..e4cf535 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -1239,7 +1239,7 @@ nla_put_failure:
>
> #define MAX_ACTIONS_BUFSIZE (32 * 1024)
>
> -struct sw_flow_actions *ovs_nla_alloc_flow_actions(int size)
> +static struct sw_flow_actions *nla_alloc_flow_actions(int size)
> {
> struct sw_flow_actions *sfa;
>
> @@ -1293,7 +1293,7 @@ static struct nlattr *reserve_sfa_size(struct
> sw_flow_actions **sfa,
> new_acts_size = MAX_ACTIONS_BUFSIZE;
> }
>
> - acts = ovs_nla_alloc_flow_actions(new_acts_size);
> + acts = nla_alloc_flow_actions(new_acts_size);
> if (IS_ERR(acts))
> return (void *)acts;
>
> @@ -1360,7 +1360,7 @@ static inline void add_nested_action_end(struct
> sw_flow_actions *sfa,
> a->nla_len = sfa->actions_len - st_offset;
> }
>
> -static int ovs_nla_copy_actions__(const st
Re: [ovs-dev] [PATCH] datapath: Optimize recirc action.
On Thu, Aug 7, 2014 at 2:01 PM, Andy Zhou wrote:
> On Thu, Aug 7, 2014 at 12:32 PM, Pravin B Shelar wrote:
>> OVS need to flow key for flow lookup in recic action. OVS
>> does key extract in recic action. Most of cases we could
>> use OVS_CB packet key directly and can avoid packet flow key
>> extract. SET action we can update flow-key along with packet
>> to keep it consistent. But there are some action like MPLS
>> pop which forces OVS to do flow-extract. In such cases we
>> can mark flow key as invalid so that subsequent recirc
>> action can do full flow extract.
>
> Does this optimization apply to sample action as well?
>
Good catch. sample action needs its own packet key.
>>
>> Signed-off-by: Pravin B Shelar
>> ---
>> datapath/actions.c | 197
>> +++--
>> 1 file changed, 160 insertions(+), 37 deletions(-)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index efc64f1..3c004a8 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -40,6 +40,90 @@
>> #include "vlan.h"
>> #include "vport.h"
>>
>> +static void flow_key_set_priority(struct sk_buff *skb, u32 priority)
>
> Why not just pass in pkt_key, instead of skb?
I want to hide OVS_CB(skb)->pkt_key inside these functions. I think it
improves readability.
>
>> +{
>> + OVS_CB(skb)->pkt_key->phy.priority = priority;
>> +}
>> +
>> +static void flow_key_set_skb_mark(struct sk_buff *skb, u32 skb_mark)
>> +{
>> + OVS_CB(skb)->pkt_key->phy.skb_mark = skb_mark;
>> +}
>> +
>> +static void flow_key_set_eth_src(struct sk_buff *skb, const u8 addr[])
>> +{
>> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.src, addr);
>> +}
>> +
>> +static void flow_key_set_eth_dst(struct sk_buff *skb, const u8 addr[])
>> +{
>> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.dst, addr);
>> +}
>> +
>> +static void flow_key_set_vlan_tci(struct sk_buff *skb, __be16 tci)
>> +{
>> + OVS_CB(skb)->pkt_key->eth.tci = tci;
>> +}
>> +
>> +static void flow_key_set_ipv4_src(struct sk_buff *skb, __be32 addr)
>> +{
>> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
>> +}
>> +
>> +static void flow_key_set_ipv4_dst(struct sk_buff *skb, __be32 addr)
>> +{
>> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
>> +}
>> +
>> +static void flow_key_set_ipv4_tos(struct sk_buff *skb, const struct iphdr
>> *nh)
>> +{
>> + OVS_CB(skb)->pkt_key->ip.tos = nh->tos;
>> +}
>> +
>> +static void flow_key_set_ip_ttl(struct sk_buff *skb, u8 ttl)
>> +{
>> + OVS_CB(skb)->pkt_key->ip.ttl = ttl;
>> +}
>> +
>> +static void flow_key_set_ipv6_src(struct sk_buff *skb,
>> + const __be32 addr[4])
>> +{
>> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.src, addr,
>> sizeof(__be32[4]));
>> +}
>> +
>> +static void flow_key_set_ipv6_dst(struct sk_buff *skb,
>> + const __be32 addr[4])
>> +{
>> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.dst, addr,
>> sizeof(__be32[4]));
>> +}
>> +
>> +static void flow_key_set_ipv6_fl(struct sk_buff *skb,
>> +const struct ipv6hdr *nh)
>> +{
>> + OVS_CB(skb)->pkt_key->ipv6.label = *(__be32 *)nh &
>> + htonl(IPV6_FLOWINFO_FLOWLABEL);
>> +}
>> +
>> +static void flow_key_set_tp_src(struct sk_buff *skb, __be16 port)
>> +{
>> + OVS_CB(skb)->pkt_key->tp.src = port;
>> +}
>> +
>> +static void flow_key_set_tp_dst(struct sk_buff *skb, __be16 port)
>> +{
>> + OVS_CB(skb)->pkt_key->tp.dst = port;
>> +}
>> +
>> +static void invalidate_skb_flow_key(struct sk_buff *skb)
>> +{
>> + OVS_CB(skb)->pkt_key->eth.type = 0;
>> +}
>> +
>> +static bool is_skb_flow_key_valid(struct sk_buff *skb)
>> +{
>> + return !!OVS_CB(skb)->pkt_key->eth.type;
>> +}
>> +
>> static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>> const struct nlattr *attr, int len);
>>
>> @@ -90,6 +174,7 @@ static int push_mpls(struct sk_buff *skb,
>> if (!ovs_skb_get_inner_protocol(skb))
>> ovs_skb_set_inner_protocol(skb, skb->protocol);
>> skb->protocol = mpls->mpls_ethertype;
>> + invalidate_skb_flow_key(skb);
>> return 0;
>> }
>>
>> @@ -120,6 +205,7 @@ static int pop_mpls(struct sk_buff *skb, const __be16
>> ethertype)
>> hdr->h_proto = ethertype;
>> if (eth_p_mpls(skb->protocol))
>> skb->protocol = ethertype;
>> + invalidate_skb_flow_key(skb);
>> return 0;
>> }
>>
>> @@ -139,7 +225,7 @@ static int set_mpls(struct sk_buff *skb, const __be32
>> *mpls_lse)
>> }
>>
>> *stack = *mpls_lse;
>> -
>> + invalidate_skb_flow_key(skb);
>> return 0;
>> }
>>
>> @@ -189,9 +275,12 @@ static int pop_vlan(struct sk_buff *skb)
>> }
>> /* move next vlan tag to hw accel tag */
>> if (likely(skb->protocol != htons(ETH_P_8021Q) ||
>> - skb->len < VLAN_ETH_HLEN))
>> +
Re: [ovs-dev] [PATCH 1/9] ofp-util: Remove prototypes for unimplemented functions.
Acked-by: Jarno Rajahalme
On Aug 4, 2014, at 9:21 AM, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff
> ---
> lib/ofp-util.h | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index 23396bb..39e50ed 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -687,9 +687,6 @@ void ofputil_append_table_features_reply(
> const struct ofputil_table_features *tf,
> struct list *replies);
>
> -uint16_t table_feature_prop_get_size(enum ofp13_table_feature_prop_type
> type);
> -char *table_feature_prop_get_name(enum ofp13_table_feature_prop_type type);
> -
> /* Meter band configuration for all supported band types. */
> struct ofputil_meter_band {
> uint16_t type;
> --
> 1.9.1
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpdk: Update documentation.
On Thu, Aug 07, 2014 at 01:46:52PM -0700, Pravin Shelar wrote: > On Thu, Aug 7, 2014 at 12:55 PM, Ben Pfaff wrote: > > On Thu, Aug 07, 2014 at 12:40:34PM -0700, Pravin B Shelar wrote: > >> Add usage info for ovs-vswitchd dpdk option. Update > >> INSTALL.DPDK file. > >> > >> Reported-by: Hari Sasank Bhamidipalli > >> Signed-off-by: Pravin B Shelar > > > > I see that this references INSTALL.DPDK from the --help message. It > > looks like there's only a few sentences in INSTALL.DPDK on the --dpdk > > option. To me, that makes it seem like it would be easy enough to add > > those sentences to ovs-vswitchd(8), and then people would easily find > > the documentation in the expected place. > > I thought abut that. > But just having --dpdk option does not work. User will have to refer > INSTALL.DPDK doc for other setup steps. So rather than reading two > docs, user can have all information about dpdk setup in INSTALL.DPDK. > > I can add a pointer to INSTALL.DPDK in man page, what do you think? I don't think INSTALL.DPDK is packaged in any distro, so pointing to that file doesn't help much if you got ovs from .deb or .rpm packages. Perhaps dpdk setup deserves a man-page itself and the --dpdk help could point to that man-page. fbl ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2] datapath: Optimize recirc action.
OVS need to flow key for flow lookup in recic action. OVS
does key extract in recic action. Most of cases we could
use OVS_CB packet key directly and can avoid packet flow key
extract. SET action we can update flow-key along with packet
to keep it consistent. But there are some action like MPLS
pop which forces OVS to do flow-extract. In such cases we
can mark flow key as invalid so that subsequent recirc
action can do full flow extract.
Signed-off-by: Pravin B Shelar
---
Fixed sample action.
avoid extract in mpls set and vlan push for single vlan case.
refactor recirc execute.
update tos in ipv6 set.
---
datapath/actions.c | 222 -
1 file changed, 185 insertions(+), 37 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index efc64f1..4ca86f4 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -40,6 +40,95 @@
#include "vlan.h"
#include "vport.h"
+static void flow_key_set_priority(struct sk_buff *skb, u32 priority)
+{
+ OVS_CB(skb)->pkt_key->phy.priority = priority;
+}
+
+static void flow_key_set_skb_mark(struct sk_buff *skb, u32 skb_mark)
+{
+ OVS_CB(skb)->pkt_key->phy.skb_mark = skb_mark;
+}
+
+static void flow_key_set_eth_src(struct sk_buff *skb, const u8 addr[])
+{
+ ether_addr_copy(OVS_CB(skb)->pkt_key->eth.src, addr);
+}
+
+static void flow_key_set_eth_dst(struct sk_buff *skb, const u8 addr[])
+{
+ ether_addr_copy(OVS_CB(skb)->pkt_key->eth.dst, addr);
+}
+
+static void flow_key_set_vlan_tci(struct sk_buff *skb, __be16 tci)
+{
+ OVS_CB(skb)->pkt_key->eth.tci = tci;
+}
+
+static void flow_key_set_mpls_top_lse(struct sk_buff *skb, __be32 top_lse)
+{
+ OVS_CB(skb)->pkt_key->mpls.top_lse = top_lse;
+}
+
+static void flow_key_set_ipv4_src(struct sk_buff *skb, __be32 addr)
+{
+ OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
+}
+
+static void flow_key_set_ipv4_dst(struct sk_buff *skb, __be32 addr)
+{
+ OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
+}
+
+static void flow_key_set_ip_tos(struct sk_buff *skb, u8 tos)
+{
+ OVS_CB(skb)->pkt_key->ip.tos = tos;
+}
+
+static void flow_key_set_ip_ttl(struct sk_buff *skb, u8 ttl)
+{
+ OVS_CB(skb)->pkt_key->ip.ttl = ttl;
+}
+
+static void flow_key_set_ipv6_src(struct sk_buff *skb,
+ const __be32 addr[4])
+{
+ memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.src, addr, sizeof(__be32[4]));
+}
+
+static void flow_key_set_ipv6_dst(struct sk_buff *skb,
+ const __be32 addr[4])
+{
+ memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.dst, addr, sizeof(__be32[4]));
+}
+
+static void flow_key_set_ipv6_fl(struct sk_buff *skb,
+const struct ipv6hdr *nh)
+{
+ OVS_CB(skb)->pkt_key->ipv6.label = *(__be32 *)nh &
+ htonl(IPV6_FLOWINFO_FLOWLABEL);
+}
+
+static void flow_key_set_tp_src(struct sk_buff *skb, __be16 port)
+{
+ OVS_CB(skb)->pkt_key->tp.src = port;
+}
+
+static void flow_key_set_tp_dst(struct sk_buff *skb, __be16 port)
+{
+ OVS_CB(skb)->pkt_key->tp.dst = port;
+}
+
+static void invalidate_skb_flow_key(struct sk_buff *skb)
+{
+ OVS_CB(skb)->pkt_key->eth.type = htons(0);
+}
+
+static bool is_skb_flow_key_valid(struct sk_buff *skb)
+{
+ return !!OVS_CB(skb)->pkt_key->eth.type;
+}
+
static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
const struct nlattr *attr, int len);
@@ -90,6 +179,7 @@ static int push_mpls(struct sk_buff *skb,
if (!ovs_skb_get_inner_protocol(skb))
ovs_skb_set_inner_protocol(skb, skb->protocol);
skb->protocol = mpls->mpls_ethertype;
+ invalidate_skb_flow_key(skb);
return 0;
}
@@ -120,6 +210,7 @@ static int pop_mpls(struct sk_buff *skb, const __be16
ethertype)
hdr->h_proto = ethertype;
if (eth_p_mpls(skb->protocol))
skb->protocol = ethertype;
+ invalidate_skb_flow_key(skb);
return 0;
}
@@ -139,7 +230,7 @@ static int set_mpls(struct sk_buff *skb, const __be32
*mpls_lse)
}
*stack = *mpls_lse;
-
+ flow_key_set_mpls_top_lse(skb, *stack);
return 0;
}
@@ -189,9 +280,12 @@ static int pop_vlan(struct sk_buff *skb)
}
/* move next vlan tag to hw accel tag */
if (likely(skb->protocol != htons(ETH_P_8021Q) ||
- skb->len < VLAN_ETH_HLEN))
+ skb->len < VLAN_ETH_HLEN)) {
+ flow_key_set_vlan_tci(skb, 0);
return 0;
+ }
+ invalidate_skb_flow_key(skb);
err = __pop_vlan_tci(skb, &tci);
if (unlikely(err))
return err;
@@ -218,6 +312,9 @@ static int push_vlan(struct sk_buff *skb, const struct
ovs_action_push_vlan *vla
skb->csum = csum_add(skb->csum, csum_partial(skb->data
+ (2 * ETH_ALEN), VLAN_HLEN, 0));
+
Re: [ovs-dev] [PATCH] datapath: Refactor action alloc and copy api.
On Thu, Aug 7, 2014 at 2:58 PM, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme
>
I pushed it to master.
Thanks.
> On Aug 7, 2014, at 12:51 PM, Pravin B Shelar wrote:
>
>> There are two separate API to allocate and copy actions list. Anytime
>> OVS needs to copy action list, it needs to call both functions.
>> Following patch moves action allocation to copy function to avoid
>> code duplication.
>>
>> Signed-off-by: Pravin B Shelar
>> ---
>> datapath/datapath.c | 49
>> +
>> datapath/flow_netlink.c | 24 +---
>> datapath/flow_netlink.h | 1 -
>> 3 files changed, 26 insertions(+), 48 deletions(-)
>>
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 19d41c8..62e3c26 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -569,17 +569,12 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
>> struct genl_info *info)
>> if (err)
>> goto err_flow_free;
>>
>> - acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_PACKET_ATTR_ACTIONS]));
>> - err = PTR_ERR(acts);
>> - if (IS_ERR(acts))
>> - goto err_flow_free;
>> -
>> err = ovs_nla_copy_actions(a[OVS_PACKET_ATTR_ACTIONS],
>> &flow->key, &acts);
>> - rcu_assign_pointer(flow->sf_acts, acts);
>> if (err)
>> goto err_flow_free;
>>
>> + rcu_assign_pointer(flow->sf_acts, acts);
>> OVS_CB(packet)->flow = flow;
>> OVS_CB(packet)->pkt_key = &flow->key;
>> OVS_CB(skb)->egress_tun_info = NULL;
>> @@ -899,11 +894,6 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
>> genl_info *info)
>> ovs_flow_mask_key(&new_flow->key, &new_flow->unmasked_key, &mask);
>>
>> /* Validate actions. */
>> - acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_FLOW_ATTR_ACTIONS]));
>> - error = PTR_ERR(acts);
>> - if (IS_ERR(acts))
>> - goto err_kfree_flow;
>> -
>> error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &new_flow->key,
>>&acts);
>> if (error) {
>> @@ -1000,29 +990,6 @@ error:
>> return error;
>> }
>>
>> -static struct sw_flow_actions *get_flow_actions(const struct nlattr *a,
>> - const struct sw_flow_key *key,
>> - const struct sw_flow_mask
>> *mask)
>> -{
>> - struct sw_flow_actions *acts;
>> - struct sw_flow_key masked_key;
>> - int error;
>> -
>> - acts = ovs_nla_alloc_flow_actions(nla_len(a));
>> - if (IS_ERR(acts))
>> - return acts;
>> -
>> - ovs_flow_mask_key(&masked_key, key, mask);
>> - error = ovs_nla_copy_actions(a, &masked_key, &acts);
>> - if (error) {
>> - OVS_NLERR("Flow actions may not be safe on all matching
>> packets.\n");
>> - kfree(acts);
>> - return ERR_PTR(error);
>> - }
>> -
>> - return acts;
>> -}
>> -
>> static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
>> {
>> struct nlattr **a = info->attrs;
>> @@ -1051,15 +1018,17 @@ static int ovs_flow_cmd_set(struct sk_buff *skb,
>> struct genl_info *info)
>>
>> /* Validate actions. */
>> if (a[OVS_FLOW_ATTR_ACTIONS]) {
>> - acts = get_flow_actions(a[OVS_FLOW_ATTR_ACTIONS], &key, &mask);
>> - if (IS_ERR(acts)) {
>> - error = PTR_ERR(acts);
>> + struct sw_flow_key masked_key;
>> +
>> + ovs_flow_mask_key(&masked_key, &key, &mask);
>> + error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
>> + &masked_key, &acts);
>> + if (error) {
>> + OVS_NLERR("Flow actions may not be safe on all
>> matching packets.\n");
>> goto error;
>> }
>> - }
>>
>> - /* Can allocate before locking if have acts. */
>> - if (acts) {
>> + /* Can allocate before locking if have acts. */
>> reply = ovs_flow_cmd_alloc_info(acts, info, false);
>> if (IS_ERR(reply)) {
>> error = PTR_ERR(reply);
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index 75172de..e4cf535 100644
>> --- a/datapath/flow_netlink.c
>> +++ b/datapath/flow_netlink.c
>> @@ -1239,7 +1239,7 @@ nla_put_failure:
>>
>> #define MAX_ACTIONS_BUFSIZE (32 * 1024)
>>
>> -struct sw_flow_actions *ovs_nla_alloc_flow_actions(int size)
>> +static struct sw_flow_actions *nla_alloc_flow_actions(int size)
>> {
>> struct sw_flow_actions *sfa;
>>
>> @@ -1293,7 +1293,7 @@ static struct nlattr *reserve_sfa_size(struct
>> sw_flow_actions **sfa,
>> new_acts_size = MAX_ACTIONS_BUFSIZE;
>> }
>>
>> - acts = ovs_nla_alloc_flow_actions(new_acts_size);
>> + acts = nla_alloc_flow_actions(new_acts_size);
>> if (IS_ERR(acts))
>> return
Re: [ovs-dev] [PATCH 2/9] ofp-actions: Add action bitmap abstraction.
Acked-by: Jarno Rajahalme
Three minor comments below,
Jarno
On Aug 4, 2014, at 9:21 AM, Ben Pfaff wrote:
> Until now, sets of actions have been abstracted separately outside
> ofp-actions, as enum ofputil_action_bitmap. Drawing sets of actions into
> ofp-actions, as done in this commit, makes for a better overall
> abstraction of actions, with better consistency.
>
> A big part of this commit is shifting from using ofp12_table_stats as if
> it were an abstraction for OpenFlow table stats, toward using a new
> struct ofputil_table_stats, which is what we generally do with other
> OpenFlow structures and fits better with the rest of the code.
>
> Signed-off-by: Ben Pfaff
> ---
> lib/ofp-actions.c | 156 +
> lib/ofp-actions.h | 145 +++
> lib/ofp-print.c| 105 -
> lib/ofp-util.c | 280 ++---
> lib/ofp-util.h | 66 ---
> ofproto/ofproto-dpif.c | 23 +---
> ofproto/ofproto-provider.h | 22 ++--
> ofproto/ofproto.c | 85 ++
> tests/ofp-print.at | 26 +++--
> tests/ofproto.at | 20 ++--
> 10 files changed, 495 insertions(+), 433 deletions(-)
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index e8fd922..3818b2d 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -3091,6 +3091,151 @@ ofpacts_put_openflow_instructions(const struct ofpact
> ofpacts[],
> }
> }
>
> +/* Sets of supported actions. */
> +
> +struct ofpact_xlate {
> +enum ofpact_type ofpact;
> +int ofpat;
This would benefit from some comments:
- ‘ofpact’ OVS internal enumeration of all supported action types.
- ‘ofpat' the corresponding action number specified in a given version of the
OpenFlow specification.
Also, this has nothing to do with ofproto-dpif-xlate, but “xlate” made me
initially think this would be related.
> +};
> +
> +static const struct ofpact_xlate *
> +get_ofpact_xlate(enum ofp_version version)
> +{
> +/* OpenFlow 1.0 actions. */
> +static const struct ofpact_xlate of10[] = {
> +{ OFPACT_OUTPUT, 0 },
> +{ OFPACT_SET_VLAN_VID, 1 },
> +{ OFPACT_SET_VLAN_PCP, 2 },
> +{ OFPACT_STRIP_VLAN, 3 },
> +{ OFPACT_SET_ETH_SRC, 4 },
> +{ OFPACT_SET_ETH_DST, 5 },
> +{ OFPACT_SET_IPV4_SRC, 6 },
> +{ OFPACT_SET_IPV4_DST, 7 },
> +{ OFPACT_SET_IP_DSCP, 8 },
> +{ OFPACT_SET_L4_SRC_PORT, 9 },
> +{ OFPACT_SET_L4_DST_PORT, 10 },
> +{ OFPACT_ENQUEUE, 11 },
> +{ 0, -1 },
> +};
> +
(snip)
> diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> index 8cb8862..5877151 100644
> --- a/lib/ofp-actions.h
> +++ b/lib/ofp-actions.h
> @@ -31,7 +31,7 @@
> * This macro is used directly only internally by this header, but the list is
> * still of interest to developers.
> *
> - * Each DEFINE_OFPACT invocation has the following parameters:
> + * Each OFPACT invocation has the following parameters:
> *
> * 1. , used below in the enum definition of OFPACT_, and
> *elsewhere.
Add: 4. string name of the action.
(snip)
> +static const char *
> +group_type_to_string(enum ofp11_group_type type)
> +{
> +switch (type) {
> +case OFPGT11_ALL: return "all";
> +case OFPGT11_SELECT: return "select";
> +case OFPGT11_INDIRECT: return "indirect";
> +case OFPGT11_FF: return "fast failover";
> +default: OVS_NOT_REACHED();
> +}
> +}
> +
> static void
> ofp_print_group_features(struct ds *string, const struct ofp_header *oh)
> {
> struct ofputil_group_features features;
> +int i;
>
> ofputil_decode_group_features_reply(oh, &features);
>
> @@ -2470,32 +2443,15 @@ ofp_print_group_features(struct ds *string, const
> struct ofp_header *oh)
> ds_put_format(string, "Capabilities: 0x%"PRIx32"\n",
> features.capabilities);
>
> -if (features.types & (1u << OFPGT11_ALL)) {
> -ds_put_format(string, "All group :\n");
> -ds_put_format(string,
> - "max_groups = %#"PRIx32"
> actions=0x%08"PRIx32"\n",
> - features.max_groups[0], features.actions[0]);
> -}
> -
> -if (features.types & (1u << OFPGT11_SELECT)) {
> -ds_put_format(string, "Select group :\n");
> -ds_put_format(string, "max_groups = %#"PRIx32" "
> - "actions=0x%08"PRIx32"\n",
> - features.max_groups[1], features.actions[1]);
> -}
> -
> -if (features.types & (1u << OFPGT11_INDIRECT)) {
> -ds_put_format(string, "Indirect group :\n");
> -ds_put_format(string, "max_groups = %#"PRIx32" "
> - "actions=0x%08"PRIx32"\n",
> - features.max_groups[2], features.actions[2]);
> -}
> -
> -if (features.types & (1u << OFPGT11_FF)) {
> -ds_put_format(string, "Fast Failover group
Re: [ovs-dev] [PATCH 3/9] ofp-actions: Add instructions bitmaps and fix related bug.
Acked-by: Jarno Rajahalme
Two small nits below,
Jarno
On Aug 4, 2014, at 9:21 AM, Ben Pfaff wrote:
> This will allow, later, to centralize all of the knowledge of instruction
> encoding inside ofp-actions.
>
> OFPIT11_ALL and OFPIT13_ALL are no longer used, so this commit removes
> them. Their definitions were wrong (they did not shift each bit into
> position correctly), so this commit is also a small bug fix.
>
> Signed-off-by: Ben Pfaff
> ---
> include/openflow/openflow-1.1.h | 4 ---
> include/openflow/openflow-1.3.h | 4 ---
> lib/ofp-actions.c | 66 +
> lib/ofp-actions.h | 4 +++
> lib/ofp-util.c | 6 ++--
> lib/ofp-util.h | 2 +-
> ofproto/ofproto-provider.h | 6 ++--
> ofproto/ofproto.c | 2 +-
> tests/ofproto.at| 2 +-
> 9 files changed, 80 insertions(+), 16 deletions(-)
>
> diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
> index bb6bcb0..5e618c9 100644
> --- a/include/openflow/openflow-1.1.h
> +++ b/include/openflow/openflow-1.1.h
> @@ -290,10 +290,6 @@ enum ofp11_instruction_type {
> OFPIT11_EXPERIMENTER = 0x /* Experimenter instruction */
> };
>
> -#define OFPIT11_ALL (OFPIT11_GOTO_TABLE | OFPIT11_WRITE_METADATA | \
> - OFPIT11_WRITE_ACTIONS | OFPIT11_APPLY_ACTIONS |\
> - OFPIT11_CLEAR_ACTIONS)
> -
> #define OFP11_INSTRUCTION_ALIGN 8
>
> /* Generic ofp_instruction structure. */
> diff --git a/include/openflow/openflow-1.3.h b/include/openflow/openflow-1.3.h
> index cc425f1..39de5b3 100644
> --- a/include/openflow/openflow-1.3.h
> +++ b/include/openflow/openflow-1.3.h
> @@ -91,10 +91,6 @@ enum ofp13_instruction_type {
> OFPIT13_METER = 6 /* Apply meter (rate limiter) */
> };
>
> -#define OFPIT13_ALL (OFPIT11_GOTO_TABLE | OFPIT11_WRITE_METADATA | \
> - OFPIT11_WRITE_ACTIONS | OFPIT11_APPLY_ACTIONS |\
> - OFPIT11_CLEAR_ACTIONS | OFPIT13_METER)
> -
> /* Instruction structure for OFPIT_METER */
> struct ofp13_instruction_meter {
> ovs_be16 type; /* OFPIT13_METER */
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 3818b2d..edcf25f 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -1646,6 +1646,72 @@ OVS_INSTRUCTIONS
> }
> }
>
> +struct ovsinst_xlate {
> +enum ovs_instruction_type ovsinst;
> +int ofpit;
> +};
It would be nice to have comment on the latter ref. OpenFlow specs. I’m not so
bothered about “xlate” this time :-)
> +
> +static const struct ovsinst_xlate *
> +get_ovsinst_xlate(enum ofp_version version)
> +{
> +/* OpenFlow 1.1 and 1.2 instructions. */
> +static const struct ovsinst_xlate of11[] = {
> +{ OVSINST_OFPIT11_GOTO_TABLE, 1 },
> +{ OVSINST_OFPIT11_WRITE_METADATA, 2 },
> +{ OVSINST_OFPIT11_WRITE_ACTIONS, 3 },
> +{ OVSINST_OFPIT11_APPLY_ACTIONS, 4 },
> +{ OVSINST_OFPIT11_CLEAR_ACTIONS, 5 },
> +{ 0, -1 },
> +};
> +
> +/* OpenFlow 1.3+ instructions. */
> +static const struct ovsinst_xlate of13[] = {
> +{ OVSINST_OFPIT11_GOTO_TABLE, 1 },
> +{ OVSINST_OFPIT11_WRITE_METADATA, 2 },
> +{ OVSINST_OFPIT11_WRITE_ACTIONS, 3 },
> +{ OVSINST_OFPIT11_APPLY_ACTIONS, 4 },
> +{ OVSINST_OFPIT11_CLEAR_ACTIONS, 5 },
> +{ OVSINST_OFPIT13_METER, 6 },
> +{ 0, -1 },
> +};
> +
> +return version < OFP13_VERSION ? of11 : of13;
> +}
> +
> +/* Converts 'ovsinst_bitmap', a bitmap whose bits correspond to OVSINST_*
> + * values, into a bitmap of instructions suitable for OpenFlow 'version'
> + * (OFP11_VERSION or later), and returns the result. */
> +ovs_be32
> +ovsinst_bitmap_to_openflow(uint32_t ovsinst_bitmap, enum ofp_version version)
> +{
> +uint32_t ofpit_bitmap = 0;
> +const struct ovsinst_xlate *x;
> +
> +for (x = get_ovsinst_xlate(version); x->ofpit >= 0; x++) {
> +if (ovsinst_bitmap & (1u << x->ovsinst)) {
> +ofpit_bitmap |= 1u << x->ofpit;
> +}
> +}
> +return htonl(ofpit_bitmap);
> +}
> +
> +/* Converts 'ofpit_bitmap', a bitmap of instructions from an OpenFlow message
> + * with the given 'version' (OFP11_VERSION or later) into a bitmap whose bits
> + * correspond to OVSINST_* values, and returns the result. */
> +uint32_t
> +ovsinst_bitmap_from_openflow(ovs_be32 ofpit_bitmap, enum ofp_version version)
> +{
> +uint32_t ovsinst_bitmap = 0;
> +const struct ovsinst_xlate *x;
> +
> +for (x = get_ovsinst_xlate(version); x->ofpit >= 0; x++) {
> +if (ofpit_bitmap & htonl(1u << x->ofpit)) {
Maybe ntohl(ofpit_bitmap) once before the loop?
> +ovsinst_bitmap |= 1u << x->ovsinst;
> +}
> +}
> +return ovsinst_bitmap;
> +}
> +
> static inline struct ofp11_instruction *
> instruction_next(const struct ofp11_instruction *inst)
> {
> diff --g
Re: [ovs-dev] [PATCH 4/9] ofproto: Correctly report table miss configuration in table stats.
Acked-by: Jarno Rajahalme
On Aug 4, 2014, at 9:21 AM, Ben Pfaff wrote:
> OFPTC11_TABLE_MISS_MASK isn't a valid value at all, let alone always the
> value in use. We should report the value actually in use, as this commit
> does.
>
> There is a remaining problem: the default table configuration is
> OFPROTO_TABLE_MISS_DEFAULT, which doesn't correspond to any particular
> OFPTC11_* value or, more precisely, corresponds to a different OFPTC11_*
> value based on the OpenFlow version. The following commit fixes that
> problem.
>
> Signed-off-by: Ben Pfaff
> ---
> ofproto/ofproto-provider.h | 2 +-
> ofproto/ofproto.c | 5 -
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index ca17319..4691d87 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -799,7 +799,7 @@ struct ofproto_class {
> *
> * - 'ovsinsts' to all instructions.
> *
> - * - 'config' to OFPTC11_TABLE_MISS_MASK.
> + * - 'config' to the table miss configuration.
> *
> * - 'max_entries' to 1,000,000.
> *
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index ac7cb13..1c9c412 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -3084,6 +3084,8 @@ handle_table_stats_request(struct ofconn *ofconn,
> */
> stats = xcalloc(p->n_tables, sizeof *stats);
> for (i = 0; i < p->n_tables; i++) {
> +unsigned int config;
> +
> stats[i].table_id = i;
> sprintf(stats[i].name, "table%"PRIuSIZE, i);
> bitmap_set_multiple(stats[i].match.bm, 0, MFF_N_IDS, 1);
> @@ -3095,7 +3097,8 @@ handle_table_stats_request(struct ofconn *ofconn,
> stats[i].metadata_match = OVS_BE64_MAX;
> stats[i].metadata_write = OVS_BE64_MAX;
> stats[i].ovsinsts = (1u << N_OVS_INSTRUCTIONS) - 1;
> -stats[i].config = OFPTC11_TABLE_MISS_MASK;
> +atomic_read(&p->tables[i].config, &config);
> +stats[i].config = config;
> stats[i].max_entries = 100; /* An arbitrary big number. */
> stats[i].active_count = classifier_count(&p->tables[i].cls);
> }
> --
> 1.9.1
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 00/16] implement OF1.3+ table features; refactor ofp-actions
v1->v2: Rebased. Added 7 patches that completely refactor OpenFlow action handling and implement the OpenFlow 1.5 (draft) Copy-Field action. Ben Pfaff (16): ofp-util: Remove prototypes for unimplemented functions. ofp-actions: Add action bitmap abstraction. ofp-actions: Add instructions bitmaps and fix related bug. ofproto: Correctly report table miss configuration in table stats. ofp-util: Abstract table miss configuration and fix related bugs. ofp-util: Fix table features decoding of multiple tables. ofp-util: Fix table features decoding of action and instruction fields. ofp-util: Fix table features decoding of match and mask. ofproto: Implement OpenFlow 1.3+ table features request. ovs-ofctl: Un-document "apply_actions" because it does not exist. ovs-ofctl: Fix error message in "parse-ofp10-actions" internal command. ofp-parse: Make string conversion functions available outside this file. ofp-actions: Pretend that OpenFlow 1.0 has instructions. ofp-actions: Centralize all OpenFlow action code for maintainability. ovs-ofctl: Generalize action and instruction test commands. ofp-actions: Add support for OpenFlow 1.5 (draft) Copy-Field action. NEWS |2 + OPENFLOW-1.1+ |3 - build-aux/extract-ofp-actions | 376 ++ include/openflow/nicira-ext.h | 1014 - include/openflow/openflow-1.0.h| 50 +- include/openflow/openflow-1.1.h| 127 - include/openflow/openflow-1.2.h| 20 - include/openflow/openflow-1.3.h| 25 - include/openflow/openflow-common.h | 76 - lib/automake.mk|8 +- lib/bundle.c | 115 +- lib/bundle.h |7 +- lib/learn.c| 212 - lib/learn.h|6 +- lib/multipath.c| 60 +- lib/multipath.h| 11 +- lib/nx-match.c | 121 - lib/nx-match.h | 19 - lib/ofp-actions.c | 8165 lib/ofp-actions.h | 298 +- lib/ofp-errors.h |3 +- lib/ofp-msgs.h |2 +- lib/ofp-parse.c| 1251 +- lib/ofp-parse.h| 17 +- lib/ofp-print.c| 511 +-- lib/ofp-util.c | 1083 +++-- lib/ofp-util.def | 104 - lib/ofp-util.h | 198 +- ofproto/connmgr.c | 13 +- ofproto/ofproto-dpif.c | 57 +- ofproto/ofproto-provider.h | 76 +- ofproto/ofproto.c | 266 +- ofproto/ofproto.h | 20 +- tests/ofp-actions.at | 176 +- tests/ofp-print.at | 133 +- tests/ofproto-dpif.at | 67 +- tests/ofproto.at | 220 +- utilities/ovs-ofctl.8.in |3 - utilities/ovs-ofctl.c | 247 +- 39 files changed, 7779 insertions(+), 7383 deletions(-) create mode 100755 build-aux/extract-ofp-actions delete mode 100644 lib/ofp-util.def -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 04/16] ofproto: Correctly report table miss configuration in table stats.
OFPTC11_TABLE_MISS_MASK isn't a valid value at all, let alone always the
value in use. We should report the value actually in use, as this commit
does.
There is a remaining problem: the default table configuration is
OFPROTO_TABLE_MISS_DEFAULT, which doesn't correspond to any particular
OFPTC11_* value or, more precisely, corresponds to a different OFPTC11_*
value based on the OpenFlow version. The following commit fixes that
problem.
Signed-off-by: Ben Pfaff
---
ofproto/ofproto-provider.h |2 +-
ofproto/ofproto.c |5 -
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index ca17319..4691d87 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -799,7 +799,7 @@ struct ofproto_class {
*
* - 'ovsinsts' to all instructions.
*
- * - 'config' to OFPTC11_TABLE_MISS_MASK.
+ * - 'config' to the table miss configuration.
*
* - 'max_entries' to 1,000,000.
*
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index ac7cb13..1c9c412 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3084,6 +3084,8 @@ handle_table_stats_request(struct ofconn *ofconn,
*/
stats = xcalloc(p->n_tables, sizeof *stats);
for (i = 0; i < p->n_tables; i++) {
+unsigned int config;
+
stats[i].table_id = i;
sprintf(stats[i].name, "table%"PRIuSIZE, i);
bitmap_set_multiple(stats[i].match.bm, 0, MFF_N_IDS, 1);
@@ -3095,7 +3097,8 @@ handle_table_stats_request(struct ofconn *ofconn,
stats[i].metadata_match = OVS_BE64_MAX;
stats[i].metadata_write = OVS_BE64_MAX;
stats[i].ovsinsts = (1u << N_OVS_INSTRUCTIONS) - 1;
-stats[i].config = OFPTC11_TABLE_MISS_MASK;
+atomic_read(&p->tables[i].config, &config);
+stats[i].config = config;
stats[i].max_entries = 100; /* An arbitrary big number. */
stats[i].active_count = classifier_count(&p->tables[i].cls);
}
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 02/16] ofp-actions: Add action bitmap abstraction.
Until now, sets of actions have been abstracted separately outside
ofp-actions, as enum ofputil_action_bitmap. Drawing sets of actions into
ofp-actions, as done in this commit, makes for a better overall
abstraction of actions, with better consistency.
A big part of this commit is shifting from using ofp12_table_stats as if
it were an abstraction for OpenFlow table stats, toward using a new
struct ofputil_table_stats, which is what we generally do with other
OpenFlow structures and fits better with the rest of the code.
Signed-off-by: Ben Pfaff
---
lib/ofp-actions.c | 156
lib/ofp-actions.h | 145 ---
lib/ofp-print.c| 105 -
lib/ofp-util.c | 280 +---
lib/ofp-util.h | 66 ---
ofproto/ofproto-dpif.c | 23 +---
ofproto/ofproto-provider.h | 22 ++--
ofproto/ofproto.c | 85 ++
tests/ofp-print.at | 26 ++--
tests/ofproto.at | 20 ++--
10 files changed, 495 insertions(+), 433 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index e8fd922..3818b2d 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -3091,6 +3091,151 @@ ofpacts_put_openflow_instructions(const struct ofpact
ofpacts[],
}
}
+/* Sets of supported actions. */
+
+struct ofpact_xlate {
+enum ofpact_type ofpact;
+int ofpat;
+};
+
+static const struct ofpact_xlate *
+get_ofpact_xlate(enum ofp_version version)
+{
+/* OpenFlow 1.0 actions. */
+static const struct ofpact_xlate of10[] = {
+{ OFPACT_OUTPUT, 0 },
+{ OFPACT_SET_VLAN_VID, 1 },
+{ OFPACT_SET_VLAN_PCP, 2 },
+{ OFPACT_STRIP_VLAN, 3 },
+{ OFPACT_SET_ETH_SRC, 4 },
+{ OFPACT_SET_ETH_DST, 5 },
+{ OFPACT_SET_IPV4_SRC, 6 },
+{ OFPACT_SET_IPV4_DST, 7 },
+{ OFPACT_SET_IP_DSCP, 8 },
+{ OFPACT_SET_L4_SRC_PORT, 9 },
+{ OFPACT_SET_L4_DST_PORT, 10 },
+{ OFPACT_ENQUEUE, 11 },
+{ 0, -1 },
+};
+
+/* OpenFlow 1.1 actions. */
+static const struct ofpact_xlate of11[] = {
+{ OFPACT_OUTPUT, 0 },
+{ OFPACT_SET_VLAN_VID, 1 },
+{ OFPACT_SET_VLAN_PCP, 2 },
+{ OFPACT_SET_ETH_SRC, 3 },
+{ OFPACT_SET_ETH_DST, 4 },
+{ OFPACT_SET_IPV4_SRC, 5 },
+{ OFPACT_SET_IPV4_DST, 6 },
+{ OFPACT_SET_IP_DSCP, 7 },
+{ OFPACT_SET_IP_ECN, 8 },
+{ OFPACT_SET_L4_SRC_PORT, 9 },
+{ OFPACT_SET_L4_DST_PORT, 10 },
+/* OFPAT_COPY_TTL_OUT (11) not supported. */
+/* OFPAT_COPY_TTL_IN (12) not supported. */
+{ OFPACT_SET_MPLS_LABEL, 13 },
+{ OFPACT_SET_MPLS_TC, 14 },
+{ OFPACT_SET_MPLS_TTL, 15 },
+{ OFPACT_DEC_MPLS_TTL, 16 },
+{ OFPACT_PUSH_VLAN, 17 },
+{ OFPACT_STRIP_VLAN, 18 },
+{ OFPACT_PUSH_MPLS, 19 },
+{ OFPACT_POP_MPLS, 20 },
+{ OFPACT_SET_QUEUE, 21 },
+{ OFPACT_GROUP, 22 },
+{ OFPACT_SET_IP_TTL, 23 },
+{ OFPACT_DEC_TTL, 24 },
+{ 0, -1 },
+};
+
+/* OpenFlow 1.2, 1.3, and 1.4 actions. */
+static const struct ofpact_xlate of12[] = {
+{ OFPACT_OUTPUT, 0 },
+/* OFPAT_COPY_TTL_OUT (11) not supported. */
+/* OFPAT_COPY_TTL_IN (12) not supported. */
+{ OFPACT_SET_MPLS_TTL, 15 },
+{ OFPACT_DEC_MPLS_TTL, 16 },
+{ OFPACT_PUSH_VLAN, 17 },
+{ OFPACT_STRIP_VLAN, 18 },
+{ OFPACT_PUSH_MPLS, 19 },
+{ OFPACT_POP_MPLS, 20 },
+{ OFPACT_SET_QUEUE, 21 },
+{ OFPACT_GROUP, 22 },
+{ OFPACT_SET_IP_TTL, 23 },
+{ OFPACT_DEC_TTL, 24 },
+{ OFPACT_SET_FIELD, 25 },
+/* OF1.3+ OFPAT_PUSH_PBB (26) not supported. */
+/* OF1.3+ OFPAT_POP_PBB (27) not supported. */
+{ 0, -1 },
+};
+
+switch (version) {
+case OFP10_VERSION:
+return of10;
+
+case OFP11_VERSION:
+return of11;
+
+case OFP12_VERSION:
+case OFP13_VERSION:
+case OFP14_VERSION:
+case OFP15_VERSION:
+default:
+return of12;
+}
+}
+
+/* Converts 'ofpacts_bitmap', a bitmap whose bits correspond to OFPACT_*
+ * values, into a bitmap of actions suitable for OpenFlow 'version', and
+ * returns the result. */
+ovs_be32
+ofpact_bitmap_to_openflow(uint64_t ofpacts_bitmap, enum ofp_version version)
+{
+uint32_t openflow_bitmap = 0;
+const struct ofpact_xlate *x;
+
+for (x = get_ofpact_xlate(version); x->ofpat >= 0; x++) {
+if (ofpacts_bitmap & (UINT64_C(1) << x->ofpact)) {
+openflow_bitmap |= 1u << x->ofpat;
+}
+}
+return htonl(openflow_bitmap);
+}
+
+/* Converts 'ofpat_bitmap', a bitmap of actions from an OpenFlow message with
+ * the given 'version' into a bitmap whose bits correspond to OFPACT_* values,
+ * and returns the result. */
+uint64_t
+ofpact_bitmap_from_openflow(ovs_be32 ofpat_bitmap
[ovs-dev] [PATCH v2 06/16] ofp-util: Fix table features decoding of multiple tables.
Table features replies can be packed back-to-back within a single
multipart reply. The code here didn't properly parse properties when this
occurred. This fixes the problem.
Signed-off-by: Ben Pfaff
---
lib/ofp-util.c |8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 7217d46..1e641bd 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -4607,6 +4607,7 @@ ofputil_decode_table_features(struct ofpbuf *msg,
{
const struct ofp_header *oh;
struct ofp13_table_features *otf;
+struct ofpbuf properties;
unsigned int len;
memset(tf, 0, sizeof *tf);
@@ -4629,7 +4630,8 @@ ofputil_decode_table_features(struct ofpbuf *msg,
if (len < sizeof *otf || len % 8 || len > ofpbuf_size(msg)) {
return OFPERR_OFPBPC_BAD_LEN;
}
-ofpbuf_pull(msg, sizeof *otf);
+ofpbuf_use_const(&properties, ofpbuf_pull(msg, len), len);
+ofpbuf_pull(&properties, sizeof *otf);
tf->table_id = otf->table_id;
if (tf->table_id == OFPTT_ALL) {
@@ -4642,12 +4644,12 @@ ofputil_decode_table_features(struct ofpbuf *msg,
tf->miss_config = ofputil_table_miss_from_config(otf->config, oh->version);
tf->max_entries = ntohl(otf->max_entries);
-while (ofpbuf_size(msg) > 0) {
+while (ofpbuf_size(&properties) > 0) {
struct ofpbuf payload;
enum ofperr error;
uint16_t type;
-error = pull_table_feature_property(msg, &payload, &type);
+error = pull_table_feature_property(&properties, &payload, &type);
if (error) {
return error;
}
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 08/16] ofp-util: Fix table features decoding of match and mask.
The call to parse_oxms() inside ofputil_decode_table_features() sets only one bit in either 'match' or 'mask' for a given field that is matchable: in 'mask' if the field is arbitrarily maskable or in 'match' otherwise. The code at the end of ofputil_decode_table_features() mishandled this, assuming that an arbitrarily matchable field would have that bit set in both. This meant that arbitrarily matchable fields were being misinterpreted as not matchable at all. This commit fixes the problem. Signed-off-by: Ben Pfaff --- lib/ofp-util.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 1ba3970..9b6ece9 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -4760,15 +4760,17 @@ ofputil_decode_table_features(struct ofpbuf *msg, /* Fix inconsistencies: * - * - Turn off 'mask' and 'wildcard' bits that are not in 'match', - * because a field must be matchable to be masked or wildcarded. + * - Turn on 'match' bits that are set in 'mask', because maskable + * fields are matchable. * * - Turn on 'wildcard' bits that are set in 'mask', because a field - * that is arbitrarily maskable can be wildcarded entirely. */ -bitmap_and(tf->mask.bm, tf->match.bm, MFF_N_IDS); -bitmap_and(tf->wildcard.bm, tf->match.bm, MFF_N_IDS); - + * that is arbitrarily maskable can be wildcarded entirely. + * + * - Turn off 'wildcard' bits that are not in 'match', because a field + * must be matchable for it to be meaningfully wildcarded. */ +bitmap_or(tf->match.bm, tf->mask.bm, MFF_N_IDS); bitmap_or(tf->wildcard.bm, tf->mask.bm, MFF_N_IDS); +bitmap_and(tf->wildcard.bm, tf->match.bm, MFF_N_IDS); return 0; } -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 07/16] ofp-util: Fix table features decoding of action and instruction fields.
Parsing of actions was wrong because OF1.3 says that non-experimenter
actions are 4 bytes and don't include padding. This fixes the problem.
Parsing of instructions seems wrong too because OF1.3 says that
non-experimenter instructions are 4 bytes (though it is not explicit about
padding as it is for actions). This fixes the problem there too.
Signed-off-by: Ben Pfaff
---
lib/ofp-util.c | 47 +++
1 file changed, 39 insertions(+), 8 deletions(-)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 1e641bd..1ba3970 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -61,12 +61,17 @@ struct ofp_prop_header {
/* Pulls a property, beginning with struct ofp_prop_header, from the beginning
* of 'msg'. Stores the type of the property in '*typep' and, if 'property' is
* nonnull, the entire property, including the header, in '*property'. Returns
- * 0 if successful, otherwise an error code. */
+ * 0 if successful, otherwise an error code.
+ *
+ * This function pulls the property's stated size padded out to a multiple of
+ * 'alignment' bytes. The common case in OpenFlow is an 'alignment' of 8, so
+ * you can use ofputil_pull_property() for that case. */
static enum ofperr
-ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf *property,
- uint16_t *typep)
+ofputil_pull_property__(struct ofpbuf *msg, struct ofpbuf *property,
+unsigned int alignment, uint16_t *typep)
{
struct ofp_prop_header *oph;
+unsigned int padded_len;
unsigned int len;
if (ofpbuf_size(msg) < sizeof *oph) {
@@ -75,7 +80,8 @@ ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf
*property,
oph = ofpbuf_data(msg);
len = ntohs(oph->len);
-if (len < sizeof *oph || ROUND_UP(len, 8) > ofpbuf_size(msg)) {
+padded_len = ROUND_UP(len, alignment);
+if (len < sizeof *oph || padded_len > ofpbuf_size(msg)) {
return OFPERR_OFPBPC_BAD_LEN;
}
@@ -83,10 +89,24 @@ ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf
*property,
if (property) {
ofpbuf_use_const(property, ofpbuf_data(msg), len);
}
-ofpbuf_pull(msg, ROUND_UP(len, 8));
+ofpbuf_pull(msg, padded_len);
return 0;
}
+/* Pulls a property, beginning with struct ofp_prop_header, from the beginning
+ * of 'msg'. Stores the type of the property in '*typep' and, if 'property' is
+ * nonnull, the entire property, including the header, in '*property'. Returns
+ * 0 if successful, otherwise an error code.
+ *
+ * This function pulls the property's stated size padded out to a multiple of
+ * 8 bytes, which is the common case for OpenFlow properties. */
+static enum ofperr
+ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf *property,
+ uint16_t *typep)
+{
+return ofputil_pull_property__(msg, property, 8, typep);
+}
+
static void PRINTF_FORMAT(2, 3)
log_property(bool loose, const char *message, ...)
{
@@ -4443,10 +4463,12 @@ ofputil_encode_port_mod(const struct ofputil_port_mod
*pm,
return b;
}
+
+/* Table features. */
static enum ofperr
pull_table_feature_property(struct ofpbuf *msg, struct ofpbuf *payload,
-uint16_t *typep)
+uint16_t *typep)
{
enum ofperr error;
@@ -4467,7 +4489,7 @@ parse_action_bitmap(struct ofpbuf *payload, enum
ofp_version ofp_version,
uint16_t type;
enum ofperr error;
-error = pull_table_feature_property(payload, NULL, &type);
+error = ofputil_pull_property__(payload, NULL, 1, &type);
if (error) {
return error;
}
@@ -4489,7 +4511,16 @@ parse_instruction_ids(struct ofpbuf *payload, bool
loose, uint32_t *insts)
enum ofperr error;
uint16_t ofpit;
-error = pull_table_feature_property(payload, NULL, &ofpit);
+/* OF1.3 and OF1.4 aren't clear about padding in the instruction IDs.
+ * It seems clear that they aren't padded to 8 bytes, though, because
+ * both standards say that "non-experimenter instructions are 4 bytes"
+ * and do not mention any padding before the first instruction ID.
+ * (There wouldn't be any point in padding to 8 bytes if the IDs were
+ * aligned on an odd 4-byte boundary.)
+ *
+ * Anyway, we just assume they're all glommed together on byte
+ * boundaries. */
+error = ofputil_pull_property__(payload, NULL, 1, &ofpit);
if (error) {
return error;
}
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 01/16] ofp-util: Remove prototypes for unimplemented functions.
Signed-off-by: Ben Pfaff
---
lib/ofp-util.h |3 ---
1 file changed, 3 deletions(-)
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 23396bb..39e50ed 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -687,9 +687,6 @@ void ofputil_append_table_features_reply(
const struct ofputil_table_features *tf,
struct list *replies);
-uint16_t table_feature_prop_get_size(enum ofp13_table_feature_prop_type type);
-char *table_feature_prop_get_name(enum ofp13_table_feature_prop_type type);
-
/* Meter band configuration for all supported band types. */
struct ofputil_meter_band {
uint16_t type;
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 03/16] ofp-actions: Add instructions bitmaps and fix related bug.
This will allow, later, to centralize all of the knowledge of instruction
encoding inside ofp-actions.
OFPIT11_ALL and OFPIT13_ALL are no longer used, so this commit removes
them. Their definitions were wrong (they did not shift each bit into
position correctly), so this commit is also a small bug fix.
Signed-off-by: Ben Pfaff
---
include/openflow/openflow-1.1.h |4 ---
include/openflow/openflow-1.3.h |4 ---
lib/ofp-actions.c | 66 +++
lib/ofp-actions.h |4 +++
lib/ofp-util.c |6 ++--
lib/ofp-util.h |2 +-
ofproto/ofproto-provider.h |6 ++--
ofproto/ofproto.c |2 +-
tests/ofproto.at|2 +-
9 files changed, 80 insertions(+), 16 deletions(-)
diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
index bb6bcb0..5e618c9 100644
--- a/include/openflow/openflow-1.1.h
+++ b/include/openflow/openflow-1.1.h
@@ -290,10 +290,6 @@ enum ofp11_instruction_type {
OFPIT11_EXPERIMENTER = 0x /* Experimenter instruction */
};
-#define OFPIT11_ALL (OFPIT11_GOTO_TABLE | OFPIT11_WRITE_METADATA | \
- OFPIT11_WRITE_ACTIONS | OFPIT11_APPLY_ACTIONS |\
- OFPIT11_CLEAR_ACTIONS)
-
#define OFP11_INSTRUCTION_ALIGN 8
/* Generic ofp_instruction structure. */
diff --git a/include/openflow/openflow-1.3.h b/include/openflow/openflow-1.3.h
index cc425f1..39de5b3 100644
--- a/include/openflow/openflow-1.3.h
+++ b/include/openflow/openflow-1.3.h
@@ -91,10 +91,6 @@ enum ofp13_instruction_type {
OFPIT13_METER = 6 /* Apply meter (rate limiter) */
};
-#define OFPIT13_ALL (OFPIT11_GOTO_TABLE | OFPIT11_WRITE_METADATA | \
- OFPIT11_WRITE_ACTIONS | OFPIT11_APPLY_ACTIONS |\
- OFPIT11_CLEAR_ACTIONS | OFPIT13_METER)
-
/* Instruction structure for OFPIT_METER */
struct ofp13_instruction_meter {
ovs_be16 type; /* OFPIT13_METER */
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 3818b2d..edcf25f 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1646,6 +1646,72 @@ OVS_INSTRUCTIONS
}
}
+struct ovsinst_xlate {
+enum ovs_instruction_type ovsinst;
+int ofpit;
+};
+
+static const struct ovsinst_xlate *
+get_ovsinst_xlate(enum ofp_version version)
+{
+/* OpenFlow 1.1 and 1.2 instructions. */
+static const struct ovsinst_xlate of11[] = {
+{ OVSINST_OFPIT11_GOTO_TABLE, 1 },
+{ OVSINST_OFPIT11_WRITE_METADATA, 2 },
+{ OVSINST_OFPIT11_WRITE_ACTIONS, 3 },
+{ OVSINST_OFPIT11_APPLY_ACTIONS, 4 },
+{ OVSINST_OFPIT11_CLEAR_ACTIONS, 5 },
+{ 0, -1 },
+};
+
+/* OpenFlow 1.3+ instructions. */
+static const struct ovsinst_xlate of13[] = {
+{ OVSINST_OFPIT11_GOTO_TABLE, 1 },
+{ OVSINST_OFPIT11_WRITE_METADATA, 2 },
+{ OVSINST_OFPIT11_WRITE_ACTIONS, 3 },
+{ OVSINST_OFPIT11_APPLY_ACTIONS, 4 },
+{ OVSINST_OFPIT11_CLEAR_ACTIONS, 5 },
+{ OVSINST_OFPIT13_METER, 6 },
+{ 0, -1 },
+};
+
+return version < OFP13_VERSION ? of11 : of13;
+}
+
+/* Converts 'ovsinst_bitmap', a bitmap whose bits correspond to OVSINST_*
+ * values, into a bitmap of instructions suitable for OpenFlow 'version'
+ * (OFP11_VERSION or later), and returns the result. */
+ovs_be32
+ovsinst_bitmap_to_openflow(uint32_t ovsinst_bitmap, enum ofp_version version)
+{
+uint32_t ofpit_bitmap = 0;
+const struct ovsinst_xlate *x;
+
+for (x = get_ovsinst_xlate(version); x->ofpit >= 0; x++) {
+if (ovsinst_bitmap & (1u << x->ovsinst)) {
+ofpit_bitmap |= 1u << x->ofpit;
+}
+}
+return htonl(ofpit_bitmap);
+}
+
+/* Converts 'ofpit_bitmap', a bitmap of instructions from an OpenFlow message
+ * with the given 'version' (OFP11_VERSION or later) into a bitmap whose bits
+ * correspond to OVSINST_* values, and returns the result. */
+uint32_t
+ovsinst_bitmap_from_openflow(ovs_be32 ofpit_bitmap, enum ofp_version version)
+{
+uint32_t ovsinst_bitmap = 0;
+const struct ovsinst_xlate *x;
+
+for (x = get_ovsinst_xlate(version); x->ofpit >= 0; x++) {
+if (ofpit_bitmap & htonl(1u << x->ofpit)) {
+ovsinst_bitmap |= 1u << x->ovsinst;
+}
+}
+return ovsinst_bitmap;
+}
+
static inline struct ofp11_instruction *
instruction_next(const struct ofp11_instruction *inst)
{
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 5877151..c215ffc 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -758,4 +758,8 @@ enum ovs_instruction_type
ovs_instruction_type_from_ofpact_type(
enum ofperr ovs_instruction_type_from_inst_type(
enum ovs_instruction_type *instruction_type, const uint16_t inst_type);
+ovs_be32 ovsinst_bitmap_to_openflow(uint32_t ovsinst_bitmap, enum ofp_version);
+uint32_t ovsinst_bitmap_from_openflow(ovs_be32 ofpit_bitmap,
+
[ovs-dev] [PATCH v2 05/16] ofp-util: Abstract table miss configuration and fix related bugs.
The ofproto implementation has had an abstraction layer on top of
OFPTC11_TABLE_MISS for a while. This commit pushes that abstraction layer
farther down, into ofp-util. This will be more useful in an upcoming
commit.
During the conversion I realized that the previous implementation was
not entirely correct. In particular, the OpenFlow 1.3+ "table mod" was
still being treated as if it had table miss configuration bits, even
though it doesn't. This commit fixes that issue and updates the tests.
OpenFlow 1.4 adds some more OFPTC_* flags that this new abstraction doesn't
yet support, but OVS didn't support those flags any better before this
commit, so abstracting those is left as future work.
Signed-off-by: Ben Pfaff
---
lib/ofp-parse.c|9 +-
lib/ofp-print.c| 408 ++
lib/ofp-util.c | 589 +---
lib/ofp-util.h | 67 +++--
ofproto/connmgr.c | 13 +-
ofproto/ofproto-dpif.c | 43 ++--
ofproto/ofproto-provider.h | 72 ++
ofproto/ofproto.c | 216 ++--
ofproto/ofproto.h | 20 +-
tests/ofp-print.at | 107 +---
tests/ofproto-dpif.at | 59 +++--
tests/ofproto.at | 109 +---
12 files changed, 1011 insertions(+), 701 deletions(-)
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index f0a30cf..2925157 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1865,16 +1865,17 @@ parse_ofp_table_mod(struct ofputil_table_mod *tm, const
char *table_id,
}
if (strcmp(flow_miss_handling, "controller") == 0) {
-tm->config = OFPTC11_TABLE_MISS_CONTROLLER;
+tm->miss_config = OFPUTIL_TABLE_MISS_CONTROLLER;
} else if (strcmp(flow_miss_handling, "continue") == 0) {
-tm->config = OFPTC11_TABLE_MISS_CONTINUE;
+tm->miss_config = OFPUTIL_TABLE_MISS_CONTINUE;
} else if (strcmp(flow_miss_handling, "drop") == 0) {
-tm->config = OFPTC11_TABLE_MISS_DROP;
+tm->miss_config = OFPUTIL_TABLE_MISS_DROP;
} else {
return xasprintf("invalid flow_miss_handling %s", flow_miss_handling);
}
-if (tm->table_id == 0xfe && tm->config == OFPTC11_TABLE_MISS_CONTINUE) {
+if (tm->table_id == 0xfe
+&& tm->miss_config == OFPUTIL_TABLE_MISS_CONTINUE) {
return xstrdup("last table's flow miss handling can not be continue");
}
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 2b0b20f..dbff150 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -51,7 +51,9 @@
static void ofp_print_queue_name(struct ds *string, uint32_t port);
static void ofp_print_error(struct ds *, enum ofperr);
-
+static void ofp_print_table_features(struct ds *,
+ const struct ofputil_table_features *,
+ const struct ofputil_table_stats *);
/* Returns a string that represents the contents of the Ethernet frame in the
* 'len' bytes starting at 'data'. The caller must free the returned string.*/
@@ -957,22 +959,21 @@ ofp_print_port_mod(struct ds *string, const struct
ofp_header *oh)
}
static void
-ofp_print_table_miss_config(struct ds *string, const uint32_t config)
+ofp_print_table_miss_config(struct ds *string, enum ofputil_table_miss miss)
{
-uint32_t table_miss_config = config & OFPTC11_TABLE_MISS_MASK;
-
-switch (table_miss_config) {
-case OFPTC11_TABLE_MISS_CONTROLLER:
+switch (miss) {
+case OFPUTIL_TABLE_MISS_CONTROLLER:
ds_put_cstr(string, "controller\n");
break;
-case OFPTC11_TABLE_MISS_CONTINUE:
+case OFPUTIL_TABLE_MISS_CONTINUE:
ds_put_cstr(string, "continue\n");
break;
-case OFPTC11_TABLE_MISS_DROP:
+case OFPUTIL_TABLE_MISS_DROP:
ds_put_cstr(string, "drop\n");
break;
+case OFPUTIL_TABLE_MISS_DEFAULT:
default:
-ds_put_cstr(string, "Unknown\n");
+ds_put_format(string, "Unknown (%d)\n", miss);
break;
}
}
@@ -995,8 +996,10 @@ ofp_print_table_mod(struct ds *string, const struct
ofp_header *oh)
ds_put_format(string, " table_id=%"PRIu8, pm.table_id);
}
-ds_put_cstr(string, ", flow_miss_config=");
-ofp_print_table_miss_config(string, pm.config);
+if (pm.miss_config != OFPUTIL_TABLE_MISS_DEFAULT) {
+ds_put_cstr(string, ", flow_miss_config=");
+ofp_print_table_miss_config(string, pm.miss_config);
+}
}
static void
@@ -1573,216 +1576,27 @@ ofp_print_ofpst_port_reply(struct ds *string, const
struct ofp_header *oh,
}
static void
-ofp_print_one_ofpst_table_reply(struct ds *string, enum ofp_version
ofp_version,
-const char *name, struct ofp12_table_stats *ts)
-{
-char name_[OFP_MAX_TABLE_NAME_LEN + 1];
-
-/* ofp13_table_stats is different */
-if (ofp_version > OFP12_VERSION) {
-return;
-}
-
-ovs_strlcpy(name_, name, sizeof name_);
-
-
[ovs-dev] [PATCH v2 10/16] ovs-ofctl: Un-document "apply_actions" because it does not exist.
"apply_actions" is assumed, any other instruction has to be specified explicitly. Signed-off-by: Ben Pfaff --- utilities/ovs-ofctl.8.in |3 --- 1 file changed, 3 deletions(-) diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index 5251c53..aafda23 100644 --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -1567,9 +1567,6 @@ keep the learned flows separate from the primary flow table 0.) .RE . .RS -.IP \fBapply_actions(\fR[\fIaction\fR][\fB,\fIaction\fR...]\fB) -Applies the specific action(s) immediately. The syntax of actions are same -to \fBactions=\fR field. . .IP \fBclear_actions\fR Clears all the actions in the action set immediately. -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 12/16] ofp-parse: Make string conversion functions available outside this file.
An upcoming commit will use them from ofp-actions.
Signed-off-by: Ben Pfaff
---
lib/ofp-parse.c | 14 +++---
lib/ofp-parse.h | 13 -
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 2925157..fa453de 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -46,7 +46,7 @@
*
* Returns NULL if successful, otherwise a malloc()'d string describing the
* error. The caller is responsible for freeing the returned string. */
-static char * WARN_UNUSED_RESULT
+char * WARN_UNUSED_RESULT
str_to_u8(const char *str, const char *name, uint8_t *valuep)
{
int value;
@@ -64,7 +64,7 @@ str_to_u8(const char *str, const char *name, uint8_t *valuep)
*
* Returns NULL if successful, otherwise a malloc()'d string describing the
* error. The caller is responsible for freeing the returned string. */
-static char * WARN_UNUSED_RESULT
+char * WARN_UNUSED_RESULT
str_to_u16(const char *str, const char *name, uint16_t *valuep)
{
int value;
@@ -80,7 +80,7 @@ str_to_u16(const char *str, const char *name, uint16_t
*valuep)
*
* Returns NULL if successful, otherwise a malloc()'d string describing the
* error. The caller is responsible for freeing the returned string. */
-static char * WARN_UNUSED_RESULT
+char * WARN_UNUSED_RESULT
str_to_u32(const char *str, uint32_t *valuep)
{
char *tail;
@@ -103,7 +103,7 @@ str_to_u32(const char *str, uint32_t *valuep)
*
* Returns NULL if successful, otherwise a malloc()'d string describing the
* error. The caller is responsible for freeing the returned string. */
-static char * WARN_UNUSED_RESULT
+char * WARN_UNUSED_RESULT
str_to_u64(const char *str, uint64_t *valuep)
{
char *tail;
@@ -127,7 +127,7 @@ str_to_u64(const char *str, uint64_t *valuep)
*
* Returns NULL if successful, otherwise a malloc()'d string describing the
* error. The caller is responsible for freeing the returned string. */
-static char * WARN_UNUSED_RESULT
+char * WARN_UNUSED_RESULT
str_to_be64(const char *str, ovs_be64 *valuep)
{
uint64_t value = 0;
@@ -144,7 +144,7 @@ str_to_be64(const char *str, ovs_be64 *valuep)
*
* Returns NULL if successful, otherwise a malloc()'d string describing the
* error. The caller is responsible for freeing the returned string. */
-static char * WARN_UNUSED_RESULT
+char * WARN_UNUSED_RESULT
str_to_mac(const char *str, uint8_t mac[6])
{
if (!ovs_scan(str, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(mac))) {
@@ -157,7 +157,7 @@ str_to_mac(const char *str, uint8_t mac[6])
*
* Returns NULL if successful, otherwise a malloc()'d string describing the
* error. The caller is responsible for freeing the returned string. */
-static char * WARN_UNUSED_RESULT
+char * WARN_UNUSED_RESULT
str_to_ip(const char *str, ovs_be32 *ip)
{
struct in_addr in_addr;
diff --git a/lib/ofp-parse.h b/lib/ofp-parse.h
index 515ccd7..16398a1 100644
--- a/lib/ofp-parse.h
+++ b/lib/ofp-parse.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -23,6 +23,7 @@
#include
#include
#include "compiler.h"
+#include "openvswitch/types.h"
struct flow;
struct ofpbuf;
@@ -86,4 +87,14 @@ char *parse_ofp_group_mod_str(struct ofputil_group_mod *,
uint16_t command,
enum ofputil_protocol *usable_protocols)
WARN_UNUSED_RESULT;
+char *str_to_u8(const char *str, const char *name, uint8_t *valuep)
+WARN_UNUSED_RESULT;
+char *str_to_u16(const char *str, const char *name, uint16_t *valuep)
+WARN_UNUSED_RESULT;
+char *str_to_u32(const char *str, uint32_t *valuep) WARN_UNUSED_RESULT;
+char *str_to_u64(const char *str, uint64_t *valuep) WARN_UNUSED_RESULT;
+char *str_to_be64(const char *str, ovs_be64 *valuep) WARN_UNUSED_RESULT;
+char *str_to_mac(const char *str, uint8_t mac[6]) WARN_UNUSED_RESULT;
+char *str_to_ip(const char *str, ovs_be32 *ip) WARN_UNUSED_RESULT;
+
#endif /* ofp-parse.h */
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 11/16] ovs-ofctl: Fix error message in "parse-ofp10-actions" internal command.
Signed-off-by: Ben Pfaff
---
utilities/ovs-ofctl.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 7b4a006..4dc547d 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2965,7 +2965,7 @@ ofctl_parse_ofp10_actions(int argc OVS_UNUSED, char
*argv[] OVS_UNUSED)
error = ofpacts_pull_openflow_actions(&of10_in, ofpbuf_size(&of10_in),
OFP10_VERSION, &ofpacts);
if (error) {
-printf("bad OF1.1 actions: %s\n\n", ofperr_get_name(error));
+printf("bad OF1.0 actions: %s\n\n", ofperr_get_name(error));
ofpbuf_uninit(&ofpacts);
ofpbuf_uninit(&of10_in);
continue;
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 09/16] ofproto: Implement OpenFlow 1.3+ table features request.
Signed-off-by: Ben Pfaff
---
NEWS |1 +
OPENFLOW-1.1+ |3 --
lib/ofp-util.c| 132 +
lib/ofp-util.h|7 ++-
ofproto/ofproto.c | 36 ++-
tests/ofproto.at | 105 ++
6 files changed, 276 insertions(+), 8 deletions(-)
diff --git a/NEWS b/NEWS
index bf7eb2f..94232b7 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,7 @@ Post-v2.3.0
release. See ovs-vswitchd(8) for details.
- OpenFlow:
* OpenFlow 1.5 (draft) extended registers are now supported.
+ * OpenFlow 1.3+ table features requests are now supported (read-only).
v2.3.0 - xx xxx
diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
index 476f79a..01adf72 100644
--- a/OPENFLOW-1.1+
+++ b/OPENFLOW-1.1+
@@ -82,9 +82,6 @@ didn't compare the specs carefully yet.)
Currently we always report OFPBRC_MULTIPART_BUFFER_OVERFLOW.
[optional for OF1.3+]
-* Add OFPMP_TABLE_FEATURES statistics. Alexander Wu has posted a
- patch series. [optional for OF1.3+]
-
* IPv6 extension header handling support. Fully implementing this
requires kernel support. This likely will take some careful and
probably time-consuming design work. The actual coding, once
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 9b6ece9..261220c 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -120,6 +120,36 @@ log_property(bool loose, const char *message, ...)
}
}
+static size_t
+start_property(struct ofpbuf *msg, uint16_t type)
+{
+size_t start_ofs = ofpbuf_size(msg);
+struct ofp_prop_header *oph;
+
+oph = ofpbuf_put_uninit(msg, sizeof *oph);
+oph->type = htons(type);
+oph->len = htons(4);/* May be updated later by end_property(). */
+return start_ofs;
+}
+
+static void
+end_property(struct ofpbuf *msg, size_t start_ofs)
+{
+struct ofp_prop_header *oph;
+
+oph = ofpbuf_at_assert(msg, start_ofs, sizeof *oph);
+oph->len = htons(ofpbuf_size(msg) - start_ofs);
+ofpbuf_padto(msg, ROUND_UP(ofpbuf_size(msg), 8));
+}
+
+static void
+put_bitmap_properties(struct ofpbuf *msg, uint64_t bitmap)
+{
+for (; bitmap; bitmap = zero_rightmost_1bit(bitmap)) {
+start_property(msg, rightmost_1bit_idx(bitmap));
+}
+}
+
/* Given the wildcard bit count in the least-significant 6 of 'wcbits', returns
* an IP netmask with a 1 in each bit that must match and a 0 in each bit that
* is wildcarded.
@@ -4801,6 +4831,108 @@ ofputil_encode_table_features_request(enum ofp_version
ofp_version)
return request;
}
+static void
+put_fields_property(struct ofpbuf *reply,
+const struct mf_bitmap *fields,
+const struct mf_bitmap *masks,
+enum ofp13_table_feature_prop_type property,
+enum ofp_version version)
+{
+size_t start_ofs;
+int field;
+
+start_ofs = start_property(reply, property);
+BITMAP_FOR_EACH_1 (field, MFF_N_IDS, fields->bm) {
+uint32_t h_oxm = mf_oxm_header(field, version);
+ovs_be32 n_oxm;
+
+if (masks && bitmap_is_set(masks->bm, field)) {
+h_oxm = NXM_MAKE_WILD_HEADER(h_oxm);
+}
+
+n_oxm = htonl(h_oxm);
+ofpbuf_put(reply, &n_oxm, sizeof n_oxm);
+}
+end_property(reply, start_ofs);
+}
+
+static void
+put_table_action_features(struct ofpbuf *reply,
+ const struct ofputil_table_action_features *taf,
+ enum ofp13_table_feature_prop_type actions_type,
+ enum ofp13_table_feature_prop_type set_fields_type,
+ int miss_offset, enum ofp_version version)
+{
+size_t start_ofs;
+
+start_ofs = start_property(reply, actions_type + miss_offset);
+put_bitmap_properties(reply,
+ ntohl(ofpact_bitmap_to_openflow(taf->ofpacts,
+ version)));
+end_property(reply, start_ofs);
+
+put_fields_property(reply, &taf->set_fields, NULL,
+set_fields_type + miss_offset, version);
+}
+
+static void
+put_table_instruction_features(
+struct ofpbuf *reply, const struct ofputil_table_instruction_features *tif,
+int miss_offset, enum ofp_version version)
+{
+size_t start_ofs;
+uint8_t table_id;
+
+start_ofs = start_property(reply, OFPTFPT13_INSTRUCTIONS + miss_offset);
+put_bitmap_properties(reply,
+ ntohl(ovsinst_bitmap_to_openflow(tif->instructions,
+ version)));
+end_property(reply, start_ofs);
+
+start_ofs = start_property(reply, OFPTFPT13_NEXT_TABLES + miss_offset);
+BITMAP_FOR_EACH_1 (table_id, 255, tif->next) {
+ofpbuf_put(reply, &table_id, 1);
+}
+end_property(reply, start_ofs);
+
+put_table_action_features(reply, &tif->write,
+
[ovs-dev] [PATCH v2 13/16] ofp-actions: Pretend that OpenFlow 1.0 has instructions.
This allows callers to be more uniform, because they don't have to pick
out whether they should parse actions or instructions based on the OpenFlow
version in use. It also allows the Write-Metadata instruction emulation
in OpenFlow 1.0 to be treated just as in OpenFlow 1.1 in the sense that
it is allowed in contexts where instructions are allowed in OpenFlow 1.1
and not elsewhere. (The changes in the tests reflect this more accurate
treatment.)
Signed-off-by: Ben Pfaff
---
lib/ofp-actions.c | 84 +++---
lib/ofp-actions.h |3 +-
lib/ofp-errors.h |3 +-
lib/ofp-parse.c |6 ++-
lib/ofp-util.c| 52 ++---
tests/ofp-actions.at | 122 ++---
utilities/ovs-ofctl.c | 33 ++---
7 files changed, 182 insertions(+), 121 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index edcf25f..fbd5b1f 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -671,28 +671,13 @@ ofpacts_from_openflow(const union ofp_action *in, size_t
n_in,
return 0;
}
-/* Attempts to convert 'actions_len' bytes of OpenFlow actions from the
- * front of 'openflow' into ofpacts. On success, replaces any existing content
- * in 'ofpacts' by the converted ofpacts; on failure, clears 'ofpacts'.
- * Returns 0 if successful, otherwise an OpenFlow error.
- *
- * Actions are processed according to their OpenFlow version which
- * is provided in the 'version' parameter.
- *
- * In most places in OpenFlow 1.1 and 1.2, actions appear encapsulated in
- * instructions, so you should call ofpacts_pull_openflow_instructions()
- * instead of this function.
- *
- * The parsed actions are valid generically, but they may not be valid in a
- * specific context. For example, port numbers up to OFPP_MAX are valid
- * generically, but specific datapaths may only support port numbers in a
- * smaller range. Use ofpacts_check() to additional check whether actions are
- * valid in a specific context. */
-enum ofperr
-ofpacts_pull_openflow_actions(struct ofpbuf *openflow,
- unsigned int actions_len,
- enum ofp_version version,
- struct ofpbuf *ofpacts) {
+static enum ofperr
+ofpacts_pull_openflow_actions__(struct ofpbuf *openflow,
+unsigned int actions_len,
+enum ofp_version version,
+uint32_t allowed_ovsinsts,
+struct ofpbuf *ofpacts)
+{
const union ofp_action *actions;
enum ofperr error;
@@ -719,13 +704,41 @@ ofpacts_pull_openflow_actions(struct ofpbuf *openflow,
return error;
}
-error = ofpacts_verify(ofpbuf_data(ofpacts), ofpbuf_size(ofpacts));
+error = ofpacts_verify(ofpbuf_data(ofpacts), ofpbuf_size(ofpacts),
+ allowed_ovsinsts);
if (error) {
ofpbuf_clear(ofpacts);
}
return error;
}
+/* Attempts to convert 'actions_len' bytes of OpenFlow actions from the
+ * front of 'openflow' into ofpacts. On success, replaces any existing content
+ * in 'ofpacts' by the converted ofpacts; on failure, clears 'ofpacts'.
+ * Returns 0 if successful, otherwise an OpenFlow error.
+ *
+ * Actions are processed according to their OpenFlow version which
+ * is provided in the 'version' parameter.
+ *
+ * In most places in OpenFlow, actions appear encapsulated in instructions, so
+ * you should call ofpacts_pull_openflow_instructions() instead of this
+ * function.
+ *
+ * The parsed actions are valid generically, but they may not be valid in a
+ * specific context. For example, port numbers up to OFPP_MAX are valid
+ * generically, but specific datapaths may only support port numbers in a
+ * smaller range. Use ofpacts_check() to additional check whether actions are
+ * valid in a specific context. */
+enum ofperr
+ofpacts_pull_openflow_actions(struct ofpbuf *openflow,
+ unsigned int actions_len,
+ enum ofp_version version,
+ struct ofpbuf *ofpacts)
+{
+return ofpacts_pull_openflow_actions__(openflow, actions_len, version,
+ 1u << OVSINST_OFPIT11_APPLY_ACTIONS,
+ ofpacts);
+}
/* OpenFlow 1.1 actions. */
@@ -1816,6 +1829,13 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
*openflow,
const struct ofp11_instruction *insts[N_OVS_INSTRUCTIONS];
enum ofperr error;
+if (version == OFP10_VERSION) {
+return ofpacts_pull_openflow_actions__(openflow, instructions_len,
+ version,
+ (1u << N_OVS_INSTRUCTIONS) - 1,
+ ofpacts);
+}
+
ofpbuf_clear(ofpacts);
if (instructions_len
[ovs-dev] [PATCH v2 16/16] ofp-actions: Add support for OpenFlow 1.5 (draft) Copy-Field action.
ONF-JIRA: EXT-320
Signed-off-by: Ben Pfaff
---
NEWS |1 +
lib/ofp-actions.c| 80 ++
tests/ofp-actions.at | 24 +++
3 files changed, 93 insertions(+), 12 deletions(-)
diff --git a/NEWS b/NEWS
index 94232b7..15abe45 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,7 @@ Post-v2.3.0
release. See ovs-vswitchd(8) for details.
- OpenFlow:
* OpenFlow 1.5 (draft) extended registers are now supported.
+ * OpenFlow 1.5 (draft) Copy-Field action is now supported.
* OpenFlow 1.3+ table features requests are now supported (read-only).
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 3799cfd..e2266cd 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -206,6 +206,11 @@ enum ofp_raw_action_type {
/* NX1.0+(7): struct nx_action_reg_load. */
NXAST_RAW_REG_LOAD,
+/* OF1.5+(28): struct ofp15_action_copy_field. */
+OFPAT_RAW15_COPY_FIELD,
+/* NX1.0-1.4(6): struct nx_action_reg_move. */
+NXAST_RAW_REG_MOVE,
+
/* ## - ## */
/* ## Nicira extension actions. ## */
/* ## - ## */
@@ -225,9 +230,6 @@ enum ofp_raw_action_type {
/* NX1.0+(5): void. */
NXAST_RAW_POP_QUEUE,
-/* NX1.0+(6): struct nx_action_reg_move. */
-NXAST_RAW_REG_MOVE,
-
/* NX1.0+(8): struct nx_action_note, ... */
NXAST_RAW_NOTE,
@@ -1622,6 +1624,26 @@ format_SET_L4_DST_PORT(const struct ofpact_l4_port *a,
struct ds *s)
ds_put_format(s, "mod_tp_dst:%d", a->port);
}
+/* Action structure for OFPAT_COPY_FIELD. */
+struct ofp15_action_copy_field {
+ovs_be16 type; /* OFPAT_COPY_FIELD. */
+ovs_be16 len; /* Length is padded to 64 bits. */
+ovs_be16 n_bits;/* Number of bits to copy. */
+ovs_be16 src_offset;/* Starting bit offset in source. */
+ovs_be16 dst_offset;/* Starting bit offset in destination. */
+ovs_be16 oxm_id_len;/* Length of oxm_ids. */
+
+/* OpenFlow allows for experimenter OXM fields whose expression is longer
+ * than a standard 32-bit OXM. Thus, in the OpenFlow specification, the
+ * following is variable-length. Open vSwitch does not yet support
+ * experimenter OXM fields, so until it does we leave these as fixed
+ * size. */
+ovs_be32 src; /* OXM for source field. */
+ovs_be32 dst; /* OXM for destination field. */
+uint8_t pad[4]; /* Must be zero. */
+};
+OFP_ASSERT(sizeof(struct ofp15_action_copy_field) == 24);
+
/* Action structure for NXAST_REG_MOVE.
*
* Copies src[src_ofs:src_ofs+n_bits] to dst[dst_ofs:dst_ofs+n_bits], where
@@ -1728,6 +1750,29 @@ struct nx_action_reg_move {
OFP_ASSERT(sizeof(struct nx_action_reg_move) == 24);
static enum ofperr
+decode_OFPAT_RAW15_COPY_FIELD(const struct ofp15_action_copy_field *oacf,
+ struct ofpbuf *ofpacts)
+{
+struct ofpact_reg_move *move;
+
+if (oacf->oxm_id_len != htons(8)) {
+/* We only support 4-byte OXM IDs so far. */
+return OFPERR_OFPBAC_BAD_LEN;
+}
+
+move = ofpact_put_REG_MOVE(ofpacts);
+move->ofpact.raw = OFPAT_RAW15_COPY_FIELD;
+move->src.field = mf_from_nxm_header(ntohl(oacf->src));
+move->src.ofs = ntohs(oacf->src_offset);
+move->src.n_bits = ntohs(oacf->n_bits);
+move->dst.field = mf_from_nxm_header(ntohl(oacf->dst));
+move->dst.ofs = ntohs(oacf->dst_offset);
+move->dst.n_bits = ntohs(oacf->n_bits);
+
+return nxm_reg_move_check(move, NULL);
+}
+
+static enum ofperr
decode_NXAST_RAW_REG_MOVE(const struct nx_action_reg_move *narm,
struct ofpbuf *ofpacts)
{
@@ -1746,17 +1791,28 @@ decode_NXAST_RAW_REG_MOVE(const struct
nx_action_reg_move *narm,
static void
encode_REG_MOVE(const struct ofpact_reg_move *move,
-enum ofp_version ofp_version OVS_UNUSED,
-struct ofpbuf *out)
+enum ofp_version ofp_version, struct ofpbuf *out)
{
-struct nx_action_reg_move *narm;
+if (ofp_version >= OFP15_VERSION) {
+struct ofp15_action_copy_field *copy;
-narm = put_NXAST_REG_MOVE(out);
-narm->n_bits = htons(move->dst.n_bits);
-narm->src_ofs = htons(move->src.ofs);
-narm->dst_ofs = htons(move->dst.ofs);
-narm->src = htonl(move->src.field->nxm_header);
-narm->dst = htonl(move->dst.field->nxm_header);
+copy = put_OFPAT15_COPY_FIELD(out);
+copy->n_bits = htons(move->dst.n_bits);
+copy->src_offset = htons(move->src.ofs);
+copy->dst_offset = htons(move->dst.ofs);
+copy->oxm_id_len = htons(8);
+copy->src = htonl(mf_oxm_header(move->src.field->id, ofp_version));
+copy->dst = htonl(mf_oxm_header(move->dst.field->id, ofp_version));
+} else {
+struct nx_action_reg_move *narm;
+
+narm = put_NXAST_REG_MOVE(out);
+narm->n_bits = htons
[ovs-dev] [PATCH v2 15/16] ovs-ofctl: Generalize action and instruction test commands.
Until now, there were four of these commands: parse-ofp10-actions, parse-ofp10-instructions, parse-ofp11-actions, parse-ofp11-instructions. This is painful to add support for new OpenFlow versions and has a ton of cut-and-paste code. This commit generalizes the code to improve on both of those points. Signed-off-by: Ben Pfaff --- tests/ofp-actions.at | 34 +++ utilities/ovs-ofctl.c | 242 ++--- 2 files changed, 86 insertions(+), 190 deletions(-) diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at index 73b7dc6..9c6f228 100644 --- a/tests/ofp-actions.at +++ b/tests/ofp-actions.at @@ -69,11 +69,11 @@ 0018 2320 0009 c426384d49c53d60 # actions=set_tunnel64:0x885f3298 0018 2320 0009 885f3298 -# bad OF1.0 actions: OFPBIC_UNSUP_INST +# bad OpenFlow10 actions: OFPBIC_UNSUP_INST & ofp_actions|WARN|write_metadata instruction not allowed here 0020 2320 0016 fedcba9876543210 -# bad OF1.0 actions: OFPBIC_UNSUP_INST +# bad OpenFlow10 actions: OFPBIC_UNSUP_INST & ofp_actions|WARN|write_metadata instruction not allowed here 0020 2320 0016 fedcba9876543210 @@ -127,7 +127,7 @@ AT_CAPTURE_FILE([input.txt]) AT_CAPTURE_FILE([expout]) AT_CAPTURE_FILE([experr]) AT_CHECK( - [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-ofp10-actions < input.txt], + [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow10 < input.txt], [0], [expout], [experr]) AT_CLEANUP @@ -157,7 +157,7 @@ AT_CAPTURE_FILE([input.txt]) AT_CAPTURE_FILE([expout]) AT_CAPTURE_FILE([experr]) AT_CHECK( - [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-ofp10-instructions < input.txt], + [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-instructions OpenFlow10 < input.txt], [0], [expout], [experr]) AT_CLEANUP @@ -235,7 +235,7 @@ 0018 2320 0009 885f3298 dnl Write-Metadata is only allowed in contexts that allow instructions. & ofp_actions|WARN|write_metadata instruction not allowed here -# bad OF1.1 actions: OFPBIC_UNSUP_INST +# bad OpenFlow11 actions: OFPBIC_UNSUP_INST 0020 2320 0016 fedcba9876543210 # actions=multipath(eth_src,50,modulo_n,1,0,NXM_NX_REG0[]) @@ -289,7 +289,7 @@ AT_CAPTURE_FILE([input.txt]) AT_CAPTURE_FILE([expout]) AT_CAPTURE_FILE([experr]) AT_CHECK( - [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-ofp11-actions < input.txt], + [ovs-ofctl '-vPATTERN:console:%c|%p|%m' parse-actions OpenFlow11 < input.txt], [0], [expout], [experr]) AT_CLEANUP @@ -325,16 +325,16 @@ dnl Check that an empty Apply-Actions instruction gets dropped. 0004 0008 dnl Duplicate instruction type: -# bad OF1.1 instructions: OFPBIC_DUP_INST +# bad OpenFlow11 instructions: OFPBIC_DUP_INST 0004 0008 0004 0008 dnl Instructions not multiple of 8 in length. & ofp_actions|WARN|OpenFlow message instructions length 9 is not a multiple of 8 -# bad OF1.1 instructions: OFPBIC_BAD_LEN +# bad OpenFlow11 instructions: OFPBIC_BAD_LEN 0004 0009 01 dnl Goto-Table instruction too long. -# bad OF1.1 instructions: OFPBIC_BAD_LEN +# bad OpenFlow11 instructions: OFPBIC_BAD_LEN 0001 0010 01 00 dnl Goto-Table 1 instruction non-zero padding @@ -343,7 +343,7 @@ dnl Goto-Table 1 instruction non-zero padding 0001 0008 01 01 dnl Goto-Table 1 instruction go back to the previous table. -# bad OF1.1 instructions: OFPBRC_BAD_TABLE_ID +# bad OpenFlow11 instructions: OFPBRC_BAD_TABLE_ID 2,0001 0008 01 00 dnl Goto-Table 1 @@ -397,15 +397,15 @@ dnl Write-Metadata with mask. 0002 0018 fedcba9876543210 ff00ff00ff00ff00 dnl Write-Metadata too short. -# bad OF1.1 instructions: OFPBIC_BAD_LEN +# bad OpenFlow11 instructions: OFPBIC_BAD_LEN 0002 0010 fedcba9876543210 dnl Write-Metadata too long. -# bad OF1.1 instructions: OFPBIC_BAD_LEN +# bad OpenFlow11 instructions: OFPBIC_BAD_LEN 0002 0020 fedcba9876543210 dnl Write-Metadata duplicated. -# bad OF1.1 instructions: OFPBIC_DUP_INST +# bad OpenFlow11 instructions: OFPBIC_DUP_INST 0002 0018 fedcba9876543210 ff00ff00ff00ff00 0002 0018 fedcba9876543210 ff00ff00ff00ff00 dnl Write-Metadata in wrong position (OpenFlow 1.1+ disregards the order @@ -465,7 +465,7 @@ dnl Check that an empty Write-Actions instruction gets dropped. 0003 0008 dnl Clear-Actions too-long -# bad OF1.1 instructions: OFPBIC_BAD_LEN +# bad OpenFlow11 instructions: OFPBIC_BAD_LEN 0005 0010 dnl Clear-Actions non-zero padding @@ -483,11 +483,11 @@ dnl Clear-Actions 0005 0008 dnl Experimenter actions not supported yet. -# bad OF1.1 instructions: OFPBIC_BAD_EXPERIMENTER +# bad OpenFlow11 instructions: OFPBIC_BAD_EXPERIMENTER 0008 01 00 dnl Bad instruction n
Re: [ovs-dev] [PATCH 4/9] ofproto: Correctly report table miss configuration in table stats.
Thanks for the reviews!
I've sent out a v2 of this series that adds 7 more patches. None of
the ones previously posted have changed, though (other than rebasing),
so you can continue to review from this version if you like.
On Thu, Aug 07, 2014 at 04:13:49PM -0700, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme
>
> On Aug 4, 2014, at 9:21 AM, Ben Pfaff wrote:
>
> > OFPTC11_TABLE_MISS_MASK isn't a valid value at all, let alone always the
> > value in use. We should report the value actually in use, as this commit
> > does.
> >
> > There is a remaining problem: the default table configuration is
> > OFPROTO_TABLE_MISS_DEFAULT, which doesn't correspond to any particular
> > OFPTC11_* value or, more precisely, corresponds to a different OFPTC11_*
> > value based on the OpenFlow version. The following commit fixes that
> > problem.
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > ofproto/ofproto-provider.h | 2 +-
> > ofproto/ofproto.c | 5 -
> > 2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index ca17319..4691d87 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -799,7 +799,7 @@ struct ofproto_class {
> > *
> > * - 'ovsinsts' to all instructions.
> > *
> > - * - 'config' to OFPTC11_TABLE_MISS_MASK.
> > + * - 'config' to the table miss configuration.
> > *
> > * - 'max_entries' to 1,000,000.
> > *
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index ac7cb13..1c9c412 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -3084,6 +3084,8 @@ handle_table_stats_request(struct ofconn *ofconn,
> > */
> > stats = xcalloc(p->n_tables, sizeof *stats);
> > for (i = 0; i < p->n_tables; i++) {
> > +unsigned int config;
> > +
> > stats[i].table_id = i;
> > sprintf(stats[i].name, "table%"PRIuSIZE, i);
> > bitmap_set_multiple(stats[i].match.bm, 0, MFF_N_IDS, 1);
> > @@ -3095,7 +3097,8 @@ handle_table_stats_request(struct ofconn *ofconn,
> > stats[i].metadata_match = OVS_BE64_MAX;
> > stats[i].metadata_write = OVS_BE64_MAX;
> > stats[i].ovsinsts = (1u << N_OVS_INSTRUCTIONS) - 1;
> > -stats[i].config = OFPTC11_TABLE_MISS_MASK;
> > +atomic_read(&p->tables[i].config, &config);
> > +stats[i].config = config;
> > stats[i].max_entries = 100; /* An arbitrary big number. */
> > stats[i].active_count = classifier_count(&p->tables[i].cls);
> > }
> > --
> > 1.9.1
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/9] ofp-util: Remove prototypes for unimplemented functions.
Thanks, applied to master.
On Thu, Aug 07, 2014 at 03:06:27PM -0700, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme
>
> On Aug 4, 2014, at 9:21 AM, Ben Pfaff wrote:
>
> > Signed-off-by: Ben Pfaff
> > ---
> > lib/ofp-util.h | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> > index 23396bb..39e50ed 100644
> > --- a/lib/ofp-util.h
> > +++ b/lib/ofp-util.h
> > @@ -687,9 +687,6 @@ void ofputil_append_table_features_reply(
> > const struct ofputil_table_features *tf,
> > struct list *replies);
> >
> > -uint16_t table_feature_prop_get_size(enum ofp13_table_feature_prop_type
> > type);
> > -char *table_feature_prop_get_name(enum ofp13_table_feature_prop_type type);
> > -
> > /* Meter band configuration for all supported band types. */
> > struct ofputil_meter_band {
> > uint16_t type;
> > --
> > 1.9.1
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 01/16] ofp-util: Remove prototypes for unimplemented functions.
On Thu, Aug 07, 2014 at 04:13:47PM -0700, Ben Pfaff wrote: > Signed-off-by: Ben Pfaff Jarno reviewed this in the v1 series, so I applied it to master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] socket-util-unix: Fix umask race in bind_unix_socket().
On Thu, Aug 07, 2014 at 02:31:24PM -0300, Flavio Leitner wrote:
> On Thu, Jul 24, 2014 at 12:53:30PM -0700, Ben Pfaff wrote:
> > The umask is a process-wide value, so bind_unix_socket() races with file
> > creation in other Open vSwitch threads. This fixes the race.
> >
> > The workaround for non-Linux systems is not ideal, but I do not know any
> > other general solution. I tested the workaround only on Linux.
>
> The Linux part looks good. Although my style preference for
> readability is like:
>
> if (LINUX) {
>/* On Linux, the fd's permissions become the file's permissions.
> fchmod() does not affect other files, like umask() does. */
>if (fchmod(fd, mode)) {
>return errno;
>}
>
>/* must be after fchmod */
>if (bind(fd, sun, sun_len))
>return errno;
>}
>
>return 0;
> }
>
> I can't tell about the specifics of FreeBSD and NetBSD though
> the work around looks sane too.
>
> Acked-by: Flavio Leitner
Fair enough. I adopted your suggested style and applied this to
master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath: Optimize recirc action.
I’ll let Andy review the recirculation part in the bottom. For set actions:
Acked-by: Jarno Rajahalme
On Aug 7, 2014, at 3:46 PM, Pravin B Shelar wrote:
> OVS need to flow key for flow lookup in recic action. OVS
> does key extract in recic action. Most of cases we could
> use OVS_CB packet key directly and can avoid packet flow key
> extract. SET action we can update flow-key along with packet
> to keep it consistent. But there are some action like MPLS
> pop which forces OVS to do flow-extract. In such cases we
> can mark flow key as invalid so that subsequent recirc
> action can do full flow extract.
>
> Signed-off-by: Pravin B Shelar
> ---
> Fixed sample action.
> avoid extract in mpls set and vlan push for single vlan case.
> refactor recirc execute.
> update tos in ipv6 set.
> ---
> datapath/actions.c | 222 -
> 1 file changed, 185 insertions(+), 37 deletions(-)
>
> diff --git a/datapath/actions.c b/datapath/actions.c
> index efc64f1..4ca86f4 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -40,6 +40,95 @@
> #include "vlan.h"
> #include "vport.h"
>
> +static void flow_key_set_priority(struct sk_buff *skb, u32 priority)
> +{
> + OVS_CB(skb)->pkt_key->phy.priority = priority;
> +}
> +
> +static void flow_key_set_skb_mark(struct sk_buff *skb, u32 skb_mark)
> +{
> + OVS_CB(skb)->pkt_key->phy.skb_mark = skb_mark;
> +}
> +
> +static void flow_key_set_eth_src(struct sk_buff *skb, const u8 addr[])
> +{
> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.src, addr);
> +}
> +
> +static void flow_key_set_eth_dst(struct sk_buff *skb, const u8 addr[])
> +{
> + ether_addr_copy(OVS_CB(skb)->pkt_key->eth.dst, addr);
> +}
> +
> +static void flow_key_set_vlan_tci(struct sk_buff *skb, __be16 tci)
> +{
> + OVS_CB(skb)->pkt_key->eth.tci = tci;
> +}
> +
> +static void flow_key_set_mpls_top_lse(struct sk_buff *skb, __be32 top_lse)
> +{
> + OVS_CB(skb)->pkt_key->mpls.top_lse = top_lse;
> +}
> +
> +static void flow_key_set_ipv4_src(struct sk_buff *skb, __be32 addr)
> +{
> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
> +}
> +
> +static void flow_key_set_ipv4_dst(struct sk_buff *skb, __be32 addr)
> +{
> + OVS_CB(skb)->pkt_key->ipv4.addr.src = addr;
> +}
> +
> +static void flow_key_set_ip_tos(struct sk_buff *skb, u8 tos)
> +{
> + OVS_CB(skb)->pkt_key->ip.tos = tos;
> +}
> +
> +static void flow_key_set_ip_ttl(struct sk_buff *skb, u8 ttl)
> +{
> + OVS_CB(skb)->pkt_key->ip.ttl = ttl;
> +}
> +
> +static void flow_key_set_ipv6_src(struct sk_buff *skb,
> + const __be32 addr[4])
> +{
> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.src, addr, sizeof(__be32[4]));
> +}
> +
> +static void flow_key_set_ipv6_dst(struct sk_buff *skb,
> + const __be32 addr[4])
> +{
> + memcpy(&OVS_CB(skb)->pkt_key->ipv6.addr.dst, addr, sizeof(__be32[4]));
> +}
> +
> +static void flow_key_set_ipv6_fl(struct sk_buff *skb,
> + const struct ipv6hdr *nh)
> +{
> + OVS_CB(skb)->pkt_key->ipv6.label = *(__be32 *)nh &
> +htonl(IPV6_FLOWINFO_FLOWLABEL);
> +}
> +
> +static void flow_key_set_tp_src(struct sk_buff *skb, __be16 port)
> +{
> + OVS_CB(skb)->pkt_key->tp.src = port;
> +}
> +
> +static void flow_key_set_tp_dst(struct sk_buff *skb, __be16 port)
> +{
> + OVS_CB(skb)->pkt_key->tp.dst = port;
> +}
> +
> +static void invalidate_skb_flow_key(struct sk_buff *skb)
> +{
> + OVS_CB(skb)->pkt_key->eth.type = htons(0);
> +}
> +
> +static bool is_skb_flow_key_valid(struct sk_buff *skb)
> +{
> + return !!OVS_CB(skb)->pkt_key->eth.type;
> +}
> +
> static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> const struct nlattr *attr, int len);
>
> @@ -90,6 +179,7 @@ static int push_mpls(struct sk_buff *skb,
> if (!ovs_skb_get_inner_protocol(skb))
> ovs_skb_set_inner_protocol(skb, skb->protocol);
> skb->protocol = mpls->mpls_ethertype;
> + invalidate_skb_flow_key(skb);
> return 0;
> }
>
> @@ -120,6 +210,7 @@ static int pop_mpls(struct sk_buff *skb, const __be16
> ethertype)
> hdr->h_proto = ethertype;
> if (eth_p_mpls(skb->protocol))
> skb->protocol = ethertype;
> + invalidate_skb_flow_key(skb);
> return 0;
> }
>
> @@ -139,7 +230,7 @@ static int set_mpls(struct sk_buff *skb, const __be32
> *mpls_lse)
> }
>
> *stack = *mpls_lse;
> -
> + flow_key_set_mpls_top_lse(skb, *stack);
> return 0;
> }
>
> @@ -189,9 +280,12 @@ static int pop_vlan(struct sk_buff *skb)
> }
> /* move next vlan tag to hw accel tag */
> if (likely(skb->protocol != htons(ETH_P_8021Q) ||
> -skb->len < VLAN_ETH_HLEN))
> +skb->len < VLAN_ETH_HLEN)) {
> + flow_key_set_vlan_tci(skb, 0);
> return 0;
> + }
>
> + invalidate_skb_flow_key(
Re: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel.
On Wed, Aug 06, 2014 at 04:30:34PM -0700, Ankur Sharma wrote: > odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel. > > Autogenerated odp-netlink.h will not compile with windows kernel, as > it refers to some userspace files like openvswitch/types.h and > packets.h which hyperv extension does not access. Due to this the > windows datapath compilation is broken on tip of tree. This patch > intends to fix that. > > In this patch we add a new sed script "extract-odp-netlink-windows-h" > to create odp-netlink-windows-dp.h. It works on similar lines as > extract-odp-netlink-h, but avoids including the header files > which are not available for driver. > > Also, added saurabh's fix to not to include some header files > in lib/netlink-protocol.h not needed by windows driver. > > After this fix, a userspace build will be needed before windows > kernel datapath can be built. > > Tested that hyperv extension could be built after building > the userspace. Verified vxlan tunnel based ping across > hypervisors. Verified that odp-netlink-windows-dp.h is not > built for linux platform. Ran 'make distcheck' to verify that > nothing is broken on linux. > > Signed-off-by: Ankur Sharma > Co-authored-by: Saurabh Shah > Tested-by: Ankur Sharma > Reported-by: Alin Serdean > Reported-by: Nithin Raju > Reported-at: https://github.com/openvswitch/ovs-issues/issues/21 I think that it would be better to generate the header file in datapath-windows/include instead of the top-level include directory, because the generated header is meant for use only from the windows datapath. That probably means that the automake.mk code should go inside datapath-windows/ as well. I'd be inclined to always build the windows datapath header, not just "if WIN32". Building it is cheap, harmless, and could possibly find bugs. I think it's probably best to move away from the windows datapath code trying to include any userspace headers, including netlink-protocol.h. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel.
On Thu, Aug 07, 2014 at 01:17:03AM +, Alin Serdean wrote: > I was wondering why not generate the #ifdef over the file that > already generates a copy of openvswitch.h (odp-netlink.h) needed for > the extension? I'm inclined to regard the userspace and the (Windows or Linux) kernel header as pretty separate. It seems just as easy at this point to just generate an appropriate header for each case. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel.
Hi Ben, Thanks a lot. I'll submit a V2 patch with the recommended changes. Regards, Ankur From: Ben Pfaff Sent: Thursday, August 7, 2014 4:40 PM To: Ankur Sharma Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel. On Wed, Aug 06, 2014 at 04:30:34PM -0700, Ankur Sharma wrote: > odp-netlink.h: Autogenerate a version of odp-netlink for windows kernel. > > Autogenerated odp-netlink.h will not compile with windows kernel, as > it refers to some userspace files like openvswitch/types.h and > packets.h which hyperv extension does not access. Due to this the > windows datapath compilation is broken on tip of tree. This patch > intends to fix that. > > In this patch we add a new sed script "extract-odp-netlink-windows-h" > to create odp-netlink-windows-dp.h. It works on similar lines as > extract-odp-netlink-h, but avoids including the header files > which are not available for driver. > > Also, added saurabh's fix to not to include some header files > in lib/netlink-protocol.h not needed by windows driver. > > After this fix, a userspace build will be needed before windows > kernel datapath can be built. > > Tested that hyperv extension could be built after building > the userspace. Verified vxlan tunnel based ping across > hypervisors. Verified that odp-netlink-windows-dp.h is not > built for linux platform. Ran 'make distcheck' to verify that > nothing is broken on linux. > > Signed-off-by: Ankur Sharma > Co-authored-by: Saurabh Shah > Tested-by: Ankur Sharma > Reported-by: Alin Serdean > Reported-by: Nithin Raju > Reported-at: > https://urldefense.proofpoint.com/v1/url?u=https://github.com/openvswitch/ovs-issues/issues/21&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=f6EhnZ0ORGZNt5QbYmRaOxfWfx%2Bqd3KEiPf3%2FYaollU%3D%0A&m=nWs7BA68Rupxq7BBS%2FwFH3cyGSymcnaK8dmHyYgDwFA%3D%0A&s=4ee3036131633e6ffd4f3fe5f8a9791ddd0e8c205c086a612feb404ef3940bbb I think that it would be better to generate the header file in datapath-windows/include instead of the top-level include directory, because the generated header is meant for use only from the windows datapath. That probably means that the automake.mk code should go inside datapath-windows/ as well. I'd be inclined to always build the windows datapath header, not just "if WIN32". Building it is cheap, harmless, and could possibly find bugs. I think it's probably best to move away from the windows datapath code trying to include any userspace headers, including netlink-protocol.h. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [DPDK Upcalls v2 1/3] flow: Parse MPLS should return the actual number of labels.
On Wed, Aug 06, 2014 at 07:40:23PM -0700, Ethan Jackson wrote: > This problem is uncovered by a future patch. > > Signed-off-by: Ethan Jackson Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpdk: Update documentation.
On Thu, Aug 07, 2014 at 07:20:41PM -0300, Flavio Leitner wrote: > On Thu, Aug 07, 2014 at 01:46:52PM -0700, Pravin Shelar wrote: > > On Thu, Aug 7, 2014 at 12:55 PM, Ben Pfaff wrote: > > > On Thu, Aug 07, 2014 at 12:40:34PM -0700, Pravin B Shelar wrote: > > >> Add usage info for ovs-vswitchd dpdk option. Update > > >> INSTALL.DPDK file. > > >> > > >> Reported-by: Hari Sasank Bhamidipalli > > >> Signed-off-by: Pravin B Shelar > > > > > > I see that this references INSTALL.DPDK from the --help message. It > > > looks like there's only a few sentences in INSTALL.DPDK on the --dpdk > > > option. To me, that makes it seem like it would be easy enough to add > > > those sentences to ovs-vswitchd(8), and then people would easily find > > > the documentation in the expected place. > > > > I thought abut that. > > But just having --dpdk option does not work. User will have to refer > > INSTALL.DPDK doc for other setup steps. So rather than reading two > > docs, user can have all information about dpdk setup in INSTALL.DPDK. > > > > I can add a pointer to INSTALL.DPDK in man page, what do you think? > > I don't think INSTALL.DPDK is packaged in any distro, so pointing to > that file doesn't help much if you got ovs from .deb or .rpm packages. > > Perhaps dpdk setup deserves a man-page itself and the --dpdk help > could point to that man-page. That's probably for the best. As an alternative, it would be easy to install INSTALL.DPDK, e.g. for Debian I think all you need is: diff --git a/debian/openvswitch-common.docs b/debian/openvswitch-common.docs index d6360a1..3bd2ca3 100644 --- a/debian/openvswitch-common.docs +++ b/debian/openvswitch-common.docs @@ -1 +1,2 @@ FAQ +INSTALL.DPDK ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpdk: Update documentation.
On Thu, Aug 07, 2014 at 04:47:02PM -0700, Ben Pfaff wrote:
> On Thu, Aug 07, 2014 at 07:20:41PM -0300, Flavio Leitner wrote:
> > On Thu, Aug 07, 2014 at 01:46:52PM -0700, Pravin Shelar wrote:
> > > On Thu, Aug 7, 2014 at 12:55 PM, Ben Pfaff wrote:
> > > > On Thu, Aug 07, 2014 at 12:40:34PM -0700, Pravin B Shelar wrote:
> > > >> Add usage info for ovs-vswitchd dpdk option. Update
> > > >> INSTALL.DPDK file.
> > > >>
> > > >> Reported-by: Hari Sasank Bhamidipalli
> > > >> Signed-off-by: Pravin B Shelar
> > > >
> > > > I see that this references INSTALL.DPDK from the --help message. It
> > > > looks like there's only a few sentences in INSTALL.DPDK on the --dpdk
> > > > option. To me, that makes it seem like it would be easy enough to add
> > > > those sentences to ovs-vswitchd(8), and then people would easily find
> > > > the documentation in the expected place.
> > >
> > > I thought abut that.
> > > But just having --dpdk option does not work. User will have to refer
> > > INSTALL.DPDK doc for other setup steps. So rather than reading two
> > > docs, user can have all information about dpdk setup in INSTALL.DPDK.
> > >
> > > I can add a pointer to INSTALL.DPDK in man page, what do you think?
> >
> > I don't think INSTALL.DPDK is packaged in any distro, so pointing to
> > that file doesn't help much if you got ovs from .deb or .rpm packages.
> >
> > Perhaps dpdk setup deserves a man-page itself and the --dpdk help
> > could point to that man-page.
>
> That's probably for the best.
>
> As an alternative, it would be easy to install INSTALL.DPDK, e.g. for
> Debian I think all you need is:
>
> diff --git a/debian/openvswitch-common.docs b/debian/openvswitch-common.docs
> index d6360a1..3bd2ca3 100644
> --- a/debian/openvswitch-common.docs
> +++ b/debian/openvswitch-common.docs
> @@ -1 +1,2 @@
> FAQ
> +INSTALL.DPDK
Here is what you need for the rpm spec files to put together
with your patch. I am adding more files because they are useful
too. Feel free to remove them if you don't agree.
Signed-off-by: Flavio Leitner
diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 30fe439..9f66f41 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -201,6 +201,7 @@ systemctl start openvswitch.service
%doc /usr/share/man/man8/ovs-test.8.gz
%doc /usr/share/man/man8/ovs-l3ping.8.gz
%doc /usr/share/man/man8/vtep-ctl.8.gz
+%doc COPYING DESIGN INSTALL.SSL NOTICE README WHY-OVS FAQ NEWS INSTALL.DPDK
/var/lib/openvswitch
/var/log/openvswitch
/usr/share/openvswitch/scripts/ovs-ctl
diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
index c817cce..251cfe5 100644
--- a/rhel/openvswitch.spec.in
+++ b/rhel/openvswitch.spec.in
@@ -55,9 +55,6 @@ rhel_cp etc_sysconfig_network-scripts_ifup-ovs 0755
rhel_cp etc_sysconfig_network-scripts_ifdown-ovs 0755
rhel_cp usr_share_openvswitch_scripts_sysconfig.template 0644
-docdir=$RPM_BUILD_ROOT/usr/share/doc/openvswitch-%{version}
-install -d -m755 "$docdir"
-install -m 0644 FAQ rhel/README.RHEL "$docdir"
install python/compat/uuid.py $RPM_BUILD_ROOT/usr/share/openvswitch/python
install python/compat/argparse.py $RPM_BUILD_ROOT/usr/share/openvswitch/python
@@ -175,7 +172,7 @@ exit 0
/usr/share/openvswitch/scripts/sysconfig.template
/usr/share/openvswitch/vswitch.ovsschema
/usr/share/openvswitch/vtep.ovsschema
-/usr/share/doc/openvswitch-%{version}/FAQ
-/usr/share/doc/openvswitch-%{version}/README.RHEL
+%doc COPYING DESIGN INSTALL.SSL NOTICE README WHY-OVS FAQ NEWS
+%doc INSTALL.DPDK rhel/README.RHEL
/var/lib/openvswitch
/var/log/openvswitch
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/3] Add BUILD_MESSAGE() macro
This commit introduces the BUILD_MESSAGE() macro. It uses _Pragma("message"),
with compilers that support that, to output a warning-like compile-time message
without blocking the compilation.
Used by next commit.
Signed-off-by: Daniele Di Proietto
---
configure.ac | 1 +
lib/compiler.h| 10 ++
m4/openvswitch.m4 | 9 +
3 files changed, 20 insertions(+)
diff --git a/configure.ac b/configure.ac
index 971c7b3..97b5cd9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -120,6 +120,7 @@ AC_ARG_VAR(KARCH, [Kernel Architecture String])
AC_SUBST(KARCH)
OVS_CHECK_LINUX
OVS_CHECK_DPDK
+OVS_CHECK_PRAGMA_MESSAGE
AC_CONFIG_FILES(Makefile)
AC_CONFIG_FILES(datapath/Makefile)
diff --git a/lib/compiler.h b/lib/compiler.h
index cfe9066..50a4739 100644
--- a/lib/compiler.h
+++ b/lib/compiler.h
@@ -220,4 +220,14 @@
#define OVS_PREFETCH_WRITE(addr)
#endif
+/* Output a message (not an error) while compiling without failing the
+ * compilation process */
+#if HAVE_PRAGMA_MESSAGE
+#define DO_PRAGMA(x) _Pragma(#x)
+#define BUILD_MESSAGE(x) \
+DO_PRAGMA(message(x))
+#else
+#define BUILD_MESSAGE(x)
+#endif
+
#endif /* compiler.h */
diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4
index 26b8058..8d6ec27 100644
--- a/m4/openvswitch.m4
+++ b/m4/openvswitch.m4
@@ -429,3 +429,12 @@ dnl OVS_CHECK_INCLUDE_NEXT
AC_DEFUN([OVS_CHECK_INCLUDE_NEXT],
[AC_REQUIRE([gl_CHECK_NEXT_HEADERS])
gl_CHECK_NEXT_HEADERS([$1])])
+
+dnl OVS_CHECK_PRAGMA_MESSAGE
+AC_DEFUN([OVS_CHECK_PRAGMA_MESSAGE],
+ [AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
+ [[_Pragma("message(\"Checking for pragma message\")")
+ ]])],
+ [AC_DEFINE(HAVE_PRAGMA_MESSAGE,1,[Define if compiler supports #pragma
+ message directive])])
+ ])
--
2.0.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 3/3] lib/flow: Use BUILD_MESSAGE() to warn if BUILD_SEQ is not updated
Signed-off-by: Daniele Di Proietto
---
lib/flow.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/flow.c b/lib/flow.c
index 2e5ca0a..78b132e 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -123,6 +123,9 @@ struct mf_ctx {
#if (FLOW_WC_SEQ != 27)
#define MINIFLOW_ASSERT(X) ovs_assert(X)
+BUILD_MESSAGE("FLOW_WC_SEQ changed: miniflow_extract() will have runtime "
+ "assertions enabled. Consider updating FLOW_WC_SEQ after "
+ "testing")
#else
#define MINIFLOW_ASSERT(X)
#endif
--
2.0.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/3] lib/flow: Update FLOW_WC_SEQ to skip assertions on miniflow_extract()
Signed-off-by: Daniele Di Proietto
---
lib/flow.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/flow.c b/lib/flow.c
index 5e04015..2e5ca0a 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -118,10 +118,10 @@ struct mf_ctx {
/* miniflow_push_* macros allow filling in a miniflow data values in order.
* Assertions are needed only when the layout of the struct flow is modified.
* 'ofs' is a compile-time constant, which allows most of the code be optimized
- * away. Some GCC versions gave warnigns on ALWAYS_INLINE, so these are
+ * away. Some GCC versions gave warnings on ALWAYS_INLINE, so these are
* defined as macros. */
-#if (FLOW_WC_SEQ != 26)
+#if (FLOW_WC_SEQ != 27)
#define MINIFLOW_ASSERT(X) ovs_assert(X)
#else
#define MINIFLOW_ASSERT(X)
--
2.0.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 0/3] Update FLOW_WC_SEQ in lib/flow.c and add "warning"
If FLOW_WC_SEQ is not updated in lib/flow.c miniflow_extract() will run with assertions enabled. This is done to help testing changes to struct flow during development, but can hurt performance a lot, since miniflow_extract() is at the heart of the userspace fast path. This patch series, beside updating the FLOW_WC_SEQ, introduces a mechanism to issue a compile-time message (without preventing the build to complete succesfully) that will prevent this kind of discrepancy in the future. Another simpler approach would be to use a build-time assertion, but that would prevent developers from testing of miniflow_extract() with run-time assertions. Daniele Di Proietto (3): lib/flow: Update FLOW_WC_SEQ to skip assertions on miniflow_extract() Add BUILD_MESSAGE() macro lib/flow: Use BUILD_MESSAGE() to warn if BUILD_SEQ is not updated configure.ac | 1 + lib/compiler.h| 10 ++ lib/flow.c| 7 +-- m4/openvswitch.m4 | 9 + 4 files changed, 25 insertions(+), 2 deletions(-) -- 2.0.1 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH RFC v2] lacp: Prefer slaves with running partner when selecting lead
On Thu, Aug 07, 2014 at 04:19:17PM +0100, Zoltan Kiss wrote: > On 05/08/14 22:16, Flavio Leitner wrote: > >On Mon, Aug 04, 2014 at 12:08:48PM -0700, Andy Zhou wrote: > >>Zoltan, > >> > >>Sorry it took a while to get back to you. I am just coming up to > >>speed on OVS LACP implementation, so my understanding may not be > >>correct. Please feel free to point them out If I am wrong. > >> > >>According to wikipeida MC-LAG entry, there is no standard for it, they > >>are mostly designed and implemented by vendors. > >> > >>After reading through the commit message, and comparing with the > >>802.1AX spec, I feel this seems like there is a bug in the MC-LAG > >>implementation/configuration issue. When the partner on port A comes > >>back again, should it wait for MC-LAG sync before using the default > >>profile to exchange states with OVS? > > > >I agree that it sounds like a problem in the MC-LAG. However, I also > >agree that OVS could do better. > > > >The aggregation selection policy is somewhat a gray area not defined > >in any spec. The bonding driver offers ad_select= parameter which > >allows to switch to the new aggregator only if, for instance, all the > >ports are down in an active aggregator. > > > >The Team driver implementing 802.3ad also provides the policy selection > >parameter. The default is to consider the prio in the LACPDU, but you > >can also tell to not select any other aggregator if the current one is > >still usable, or per bandwidth or per number of ports available. > > > >My suggestion if we want to change something is to stick with bonding > >driver default behavior regarding to select a new aggregator: > >""" > >table or 0 > > > > The active aggregator is chosen by largest aggregate > > bandwidth. > > > > Reselection of the active aggregator occurs only when all > > slaves of the active aggregator are down or the active > > aggregator has no slaves. > > > > This is the default value. > >""" > >Documentation/networking/bonding.txt > As far as I understood, there isn't really a concept of aggregator in OVS, > but "lead" in lacp_update_attached() is something similar. However it is > recalculated at every iteration, and it is simply just the slave with the > lower priority. Yes, that's my understanding as well. Rhe slave with lower priority carries the information about the partner which then is used to compare with other slaves. The ones matching with lead's partner info are in the same trunk, so they are attached, otherwise it's either not established or is in another trunk (not attached). > Do you mean we should save "lead" into a new field in struct lacp, and > select a new one only if lacp_partner_ready(lead) == false? I think it will require a bit more than that because if the "lead" slave fails for some reason, the slave is not CURRENT anymore but the LACP might be still valid because of another slave in CURRENT state, so the "lead" might need to be replaced by another slave in the same trunk to keep it up. Otherwise, the failing "lead" might receive another partner info (the default config in your case) and that would break the established trunk. Does that make sense to you? fbl ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Refactor action alloc and copy api.
On Thu, Aug 7, 2014 at 12:51 PM, Pravin B Shelar wrote: > There are two separate API to allocate and copy actions list. Anytime > OVS needs to copy action list, it needs to call both functions. > Following patch moves action allocation to copy function to avoid > code duplication. > > Signed-off-by: Pravin B Shelar This introduces a warning: CC [M] /home/jesse/openvswitch/datapath/linux/datapath.o /home/jesse/openvswitch/datapath/linux/datapath.c: In function ‘ovs_flow_cmd_set’: /home/jesse/openvswitch/datapath/linux/datapath.c:1094:1: warning: the frame size of 1256 bytes is larger than 1024 bytes [-Wframe-larger-than=] } This is essentially undoing commit 1d2a1b5f5252e4c6ce8bbf8d91ca27aba52496e6 (datapath: Factor out allocation and verification of actions.), which was supposed to avoid this problem. It's actually not a real problem but the warning is annoying. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath: Avoid using stack larger than 1024.
commit (datapath: Refactor action alloc and copy api) effectively
reverted 1d2a1b5f5252e4c6ce8bbf8d91ca27aba52496e6 (datapath: Factor out
allocation and verification of actions.). This results in following
warning:
CC [M] /home/jesse/openvswitch/datapath/linux/datapath.o
/home/jesse/openvswitch/datapath/linux/datapath.c: In function
‘ovs_flow_cmd_set’:
/home/jesse/openvswitch/datapath/linux/datapath.c:1094:1: warning: the
frame size of 1256 bytes is larger than 1024 bytes
[-Wframe-larger-than=]
}
Following patch introduced the factoring back.
CC: Jesse Gross
Signed-off-by: Pravin B Shelar
---
datapath/datapath.c | 29 ++---
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/datapath/datapath.c b/datapath/datapath.c
index f795e94..ab63791 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -990,6 +990,25 @@ error:
return error;
}
+/* Factor out action copy to avoid "Wframe-larger-than=1024" warning. */
+static struct sw_flow_actions *get_flow_actions(const struct nlattr *a,
+ const struct sw_flow_key *key,
+ const struct sw_flow_mask *mask)
+{
+ struct sw_flow_actions *acts;
+ struct sw_flow_key masked_key;
+ int error;
+
+ ovs_flow_mask_key(&masked_key, key, mask);
+ error = ovs_nla_copy_actions(a, &masked_key, &acts);
+ if (error) {
+ OVS_NLERR("Actions may not be safe on all matching packets.\n");
+ return ERR_PTR(error);
+ }
+
+ return acts;
+}
+
static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
{
struct nlattr **a = info->attrs;
@@ -1018,13 +1037,9 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct
genl_info *info)
/* Validate actions. */
if (a[OVS_FLOW_ATTR_ACTIONS]) {
- struct sw_flow_key masked_key;
-
- ovs_flow_mask_key(&masked_key, &key, &mask);
- error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
-&masked_key, &acts);
- if (error) {
- OVS_NLERR("Flow actions may not be safe on all matching
packets.\n");
+ acts = get_flow_actions(a[OVS_FLOW_ATTR_ACTIONS], &key, &mask);
+ if (IS_ERR(acts)) {
+ error = PTR_ERR(acts);
goto error;
}
--
1.9.3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Refactor action alloc and copy api.
On Thu, Aug 7, 2014 at 7:21 PM, Jesse Gross wrote: > On Thu, Aug 7, 2014 at 12:51 PM, Pravin B Shelar wrote: >> There are two separate API to allocate and copy actions list. Anytime >> OVS needs to copy action list, it needs to call both functions. >> Following patch moves action allocation to copy function to avoid >> code duplication. >> >> Signed-off-by: Pravin B Shelar > > This introduces a warning: > > CC [M] /home/jesse/openvswitch/datapath/linux/datapath.o > /home/jesse/openvswitch/datapath/linux/datapath.c: In function > ‘ovs_flow_cmd_set’: > /home/jesse/openvswitch/datapath/linux/datapath.c:1094:1: warning: the > frame size of 1256 bytes is larger than 1024 bytes > [-Wframe-larger-than=] > } > > This is essentially undoing commit > 1d2a1b5f5252e4c6ce8bbf8d91ca27aba52496e6 (datapath: Factor out > allocation and verification of actions.), which was supposed to avoid > this problem. It's actually not a real problem but the warning is > annoying. I reintroduced that commit to fix the warning. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Avoid using stack larger than 1024.
On Thu, Aug 7, 2014 at 8:34 PM, Pravin B Shelar wrote: > commit (datapath: Refactor action alloc and copy api) effectively > reverted 1d2a1b5f5252e4c6ce8bbf8d91ca27aba52496e6 (datapath: Factor out > allocation and verification of actions.). This results in following > warning: > > CC [M] /home/jesse/openvswitch/datapath/linux/datapath.o > /home/jesse/openvswitch/datapath/linux/datapath.c: In function > ‘ovs_flow_cmd_set’: > /home/jesse/openvswitch/datapath/linux/datapath.c:1094:1: warning: the > frame size of 1256 bytes is larger than 1024 bytes > [-Wframe-larger-than=] > } > > Following patch introduced the factoring back. > > CC: Jesse Gross > Signed-off-by: Pravin B Shelar Acked-by: Jesse Gross ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath: Avoid NULL mask check while building mask
OVS does mask validation even if it does not need to convert
netlink mask attributes to mask structure. ovs_nla_get_match()
caller can pass NULL mask structure pointer if the caller does
not need mask. Therefore NULL check is required in SW_FLOW_KEY*
macros. Following patch does not convert mask netlink attributes
if mask pointer is NULL, so we do not need these checks in
SW_FLOW_KEY* macro.
Signed-off-by: Pravin B Shelar
---
datapath/flow_netlink.c | 74 ++---
1 file changed, 39 insertions(+), 35 deletions(-)
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 75172de..63ab7a6 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -84,8 +84,7 @@ static void update_range__(struct sw_flow_match *match,
update_range__(match, offsetof(struct sw_flow_key, field), \
sizeof((match)->key->field), is_mask); \
if (is_mask) { \
- if ((match)->mask) \
- (match)->mask->key.field = value; \
+ (match)->mask->key.field = value; \
} else {\
(match)->key->field = value;\
} \
@@ -95,10 +94,9 @@ static void update_range__(struct sw_flow_match *match,
do { \
update_range__(match, offset, len, is_mask);\
if (is_mask) { \
- if ((match)->mask) \
- memcpy((u8 *)&(match)->mask->key + offset,
value_p, len);\
+ memcpy((u8 *)&(match)->mask->key + offset, value_p,
len);\
} else {\
- memcpy((u8 *)(match)->key + offset, value_p, len);
\
+ memcpy((u8 *)(match)->key + offset, value_p, len); \
} \
} while (0)
@@ -111,9 +109,8 @@ static void update_range__(struct sw_flow_match *match,
update_range__(match, offsetof(struct sw_flow_key, field), \
sizeof((match)->key->field), is_mask); \
if (is_mask) { \
- if ((match)->mask) \
- memset((u8 *)&(match)->mask->key.field, value,\
- sizeof((match)->mask->key.field)); \
+ memset((u8 *)&(match)->mask->key.field, value,\
+ sizeof((match)->mask->key.field)); \
} else {\
memset((u8 *)&(match)->key->field, value, \
sizeof((match)->key->field));\
@@ -633,8 +630,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
*match, u64 attrs,
SW_FLOW_KEY_PUT(match, eth.tci, tci, is_mask);
attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
- } else if (!is_mask)
- SW_FLOW_KEY_PUT(match, eth.tci, htons(0x), true);
+ }
if (attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) {
__be16 eth_type;
@@ -859,8 +855,8 @@ static void mask_set_nlattr(struct nlattr *attr, u8 val)
* attribute specifies the mask field of the wildcarded flow.
*/
int ovs_nla_get_match(struct sw_flow_match *match,
- const struct nlattr *key,
- const struct nlattr *mask)
+ const struct nlattr *nla_key,
+ const struct nlattr *nla_mask)
{
const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
const struct nlattr *encap;
@@ -870,7 +866,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
bool encap_valid = false;
int err;
- err = parse_flow_nlattrs(key, a, &key_attrs);
+ err = parse_flow_nlattrs(nla_key, a, &key_attrs);
if (err)
return err;
@@ -911,35 +907,43 @@ int ovs_nla_get_match(struct sw_flow_match *match,
if (err)
return err;
- if (match->mask && !mask) {
- /* Create an exact match mask. We need to set to 0xff all the
-* 'match->mask' fields that have been touched in 'match->key'.
-* We cannot simply memset 'match->mask', because padding bytes
-* and fields not specified in 'match->key' should be left to 0.
-* Instead, we use a stream of netlink attributes, copied fr
Re: [ovs-dev] [PATCH] datapath: Avoid using stack larger than 1024.
On Thu, Aug 7, 2014 at 9:03 PM, Jesse Gross wrote: > On Thu, Aug 7, 2014 at 8:34 PM, Pravin B Shelar wrote: >> commit (datapath: Refactor action alloc and copy api) effectively >> reverted 1d2a1b5f5252e4c6ce8bbf8d91ca27aba52496e6 (datapath: Factor out >> allocation and verification of actions.). This results in following >> warning: >> >> CC [M] /home/jesse/openvswitch/datapath/linux/datapath.o >> /home/jesse/openvswitch/datapath/linux/datapath.c: In function >> ‘ovs_flow_cmd_set’: >> /home/jesse/openvswitch/datapath/linux/datapath.c:1094:1: warning: the >> frame size of 1256 bytes is larger than 1024 bytes >> [-Wframe-larger-than=] >> } >> >> Following patch introduced the factoring back. >> >> CC: Jesse Gross >> Signed-off-by: Pravin B Shelar > > Acked-by: Jesse Gross I pushed it to master. Thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
