Re: [ovs-dev] [PATCH v4 6/7] dpif-xlate: Snoop multicast packets and send them properly
Hi Flavio!
I was recently doing some develop job on igmp snooping base on ovs, but found
failed to add route port to mrouter list.
As I looking into this patch, I found that an igmp query must have a source
network address to make the in_port adding to the mrouter
, while the igmp querier, which is also a open-source tool, doesn't have a
source network address while generating an igmp query.
So, I wonder if it's necessary to judge a query message whether it has a source
network address. If so, would you please explain why.
Looking forword you reply and best wishes.
James
于 2014/6/19 9:14, Flavio Leitner 写道:
> If the packet is multicast and the snooping feature is enabled,
> update the multicast snooping database accordingly and send it
> to the right ports.
>
> If the packet is not multicast or the snooping feature is disabled,
> let the MAC learning handle the packet as before.
>
> Acked-by: Ben Pfaff
> Signed-off-by: Flavio Leitner
> ---
> ofproto/ofproto-dpif-xlate.c | 236
> ---
> 1 file changed, 221 insertions(+), 15 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 2fc2684..52b6fe4 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1810,6 +1810,157 @@ update_learning_table(const struct xbridge *xbridge,
> }
> }
>
> +/* Updates multicast snooping table 'ms' given that a packet matching 'flow'
> + * was received on 'in_xbundle' in 'vlan' and is either Report or Query. */
> +static void
> +update_mcast_snooping_table__(const struct xbridge *xbridge,
> + const struct flow *flow,
> + struct mcast_snooping *ms,
> + ovs_be32 ip4, int vlan,
> + struct xbundle *in_xbundle)
> +OVS_REQ_WRLOCK(ms->rwlock)
> +{
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(60, 30);
> +
> +switch (ntohs(flow->tp_src)) {
> +case IGMP_HOST_MEMBERSHIP_REPORT:
> +case IGMPV2_HOST_MEMBERSHIP_REPORT:
> +if (mcast_snooping_add_group(ms, ip4, vlan, in_xbundle->ofbundle)) {
> +VLOG_DBG_RL(&rl, "bridge %s: multicast snooping learned that "
> +IP_FMT" is on port %s in VLAN %d",
> +xbridge->name, IP_ARGS(ip4), in_xbundle->name, vlan);
> +}
> +break;
> +case IGMP_HOST_LEAVE_MESSAGE:
> +if (mcast_snooping_leave_group(ms, ip4, vlan, in_xbundle->ofbundle))
> {
> +VLOG_DBG_RL(&rl, "bridge %s: multicast snooping leaving "
> +IP_FMT" is on port %s in VLAN %d",
> +xbridge->name, IP_ARGS(ip4), in_xbundle->name, vlan);
> +}
> +break;
> +case IGMP_HOST_MEMBERSHIP_QUERY:
> +if (flow->nw_src && mcast_snooping_add_mrouter(ms, vlan,
> +in_xbundle->ofbundle)) {
> +VLOG_DBG_RL(&rl, "bridge %s: multicast snooping query from "
> +IP_FMT" is on port %s in VLAN %d",
> +xbridge->name, IP_ARGS(flow->nw_src),
> +in_xbundle->name, vlan);
> +}
> +break;
> +}
> +}
> +
> +/* Updates multicast snooping table 'ms' given that a packet matching 'flow'
> + * was received on 'in_xbundle' in 'vlan'. */
> +static void
> +update_mcast_snooping_table(const struct xbridge *xbridge,
> +const struct flow *flow, int vlan,
> +struct xbundle *in_xbundle)
> +{
> +struct mcast_snooping *ms = xbridge->ms;
> +struct xlate_cfg *xcfg;
> +struct xbundle *mcast_xbundle;
> +struct mcast_fport_bundle *fport;
> +
> +/* Don't learn the OFPP_NONE port. */
> +if (in_xbundle == &ofpp_none_bundle) {
> +return;
> +}
> +
> +/* Don't learn from flood ports */
> +mcast_xbundle = NULL;
> +ovs_rwlock_wrlock(&ms->rwlock);
> +xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
> +LIST_FOR_EACH(fport, fport_node, &ms->fport_list) {
> +mcast_xbundle = xbundle_lookup(xcfg, fport->port);
> +if (mcast_xbundle == in_xbundle) {
> +break;
> +}
> +}
> +
> +if (!mcast_xbundle || mcast_xbundle != in_xbundle) {
> +update_mcast_snooping_table__(xbridge, flow, ms,
> flow->igmp_group_ip4,
> + vlan, in_xbundle);
> +}
> +ovs_rwlock_unlock(&ms->rwlock);
> +}
> +
> +/* send the packet to ports having the multicast group learned */
> +static void
> +xlate_normal_mcast_send_group(struct xlate_ctx *ctx,
> + struct mcast_snooping *ms OVS_UNUSED,
> + struct mcast_group *grp,
> + struct xbundle *in_xbundle, uint16_t vlan)
> +OVS_REQ_RDLOCK(ms->rwlock)
> +{
> +struct xlate_cfg *xcfg;
> +struct mcast_group_bundle *b;
> +struct xbundle *mcast_xbun
Re: [ovs-dev] [PATCH] datapath: Rename last_action() as nla_is_last() and move to netlink.h
On Thu, Oct 16, 2014 at 04:50:10PM -0700, Andy Zhou wrote:
> Simon, The change makes a lot of sense.
>
> I am just wondering if we should upstream the netlink.h change first?
> To me, it seems to add a new compat API that does not exist upstream.
Sure, I think that makes sense. Though I'm not sure if upstream likes
to take new API calls that aren't used. Perhaps a good way forwards
would be for me to re-submit this patch against the upstream net-next
kernel.
Pravin, how would you feel about that?
>
>
> On Wed, Sep 24, 2014 at 9:28 PM, Simon Horman
> wrote:
> > The original motivation for this change was to allow the
> > helper to be used in files other than actions.c as part
> > of work on an odp select group action.
> >
> > It was as pointed out by Thomas Graf that this helper would be best
> > off living in netlink.h. Furthermore, I think that the generic nature of
> > this
> > helper means it is best off in netlink.h regardless of if it is used more
> > than
> > one .c file or not. Thus I would like it considered independent
> > of the work on an odp select group action.
> >
> > Signed-off-by: Simon Horman
> > ---
> > datapath/actions.c | 11 +++
> > datapath/linux/compat/include/net/netlink.h | 4
> > 2 files changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/datapath/actions.c b/datapath/actions.c
> > index b527cb6..535b87e 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > @@ -671,11 +671,6 @@ static int output_userspace(struct datapath *dp,
> > struct sk_buff *skb,
> > return ovs_dp_upcall(dp, skb, key, &upcall);
> > }
> >
> > -static bool last_action(const struct nlattr *a, int rem)
> > -{
> > - return a->nla_len == rem;
> > -}
> > -
> > static int sample(struct datapath *dp, struct sk_buff *skb,
> > struct sw_flow_key *key, const struct nlattr *attr)
> > {
> > @@ -710,7 +705,7 @@ static int sample(struct datapath *dp, struct sk_buff
> > *skb,
> > * user space. This skb will be consumed by its caller.
> > */
> > if (likely(nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
> > - last_action(a, rem)))
> > + nla_is_last(a, rem)))
> > return output_userspace(dp, skb, key, a);
> >
> > skb = skb_clone(skb, GFP_ATOMIC);
> > @@ -810,7 +805,7 @@ static int execute_recirc(struct datapath *dp, struct
> > sk_buff *skb,
> > }
> > BUG_ON(!is_flow_key_valid(key));
> >
> > - if (!last_action(a, rem)) {
> > + if (!nla_is_last(a, rem)) {
> > /* Recirc action is the not the last action
> > * of the action list, need to clone the skb.
> > */
> > @@ -897,7 +892,7 @@ static int do_execute_actions(struct datapath *dp,
> > struct sk_buff *skb,
> >
> > case OVS_ACTION_ATTR_RECIRC:
> > err = execute_recirc(dp, skb, key, a, rem);
> > - if (last_action(a, rem)) {
> > + if (nla_is_last(a, rem)) {
> > /* If this is the last action, the skb has
> > * been consumed or freed.
> > * Return immediately.
> > diff --git a/datapath/linux/compat/include/net/netlink.h
> > b/datapath/linux/compat/include/net/netlink.h
> > index a6dc584..de37058 100644
> > --- a/datapath/linux/compat/include/net/netlink.h
> > +++ b/datapath/linux/compat/include/net/netlink.h
> > @@ -63,4 +63,8 @@ static inline struct nlattr *nla_find_nested(struct
> > nlattr *nla, int attrtype)
> > }
> > #endif
> >
> > +static inline bool nla_is_last(const struct nlattr *a, int rem)
> > +{
> > + return a->nla_len == rem;
> > +}
> > #endif /* net/netlink.h */
> > --
> > 2.0.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] Returned mail: see transcript for details
Your message was not delivered due to the following reason: Your message could not be delivered because the destination server was unreachable within the allowed queue period. The amount of time a message is queued before it is returned depends on local configura- tion parameters. Most likely there is a network problem that prevented delivery, but it is also possible that the computer is turned off, or does not have a mail system running right now. Your message could not be delivered within 3 days: Server 203.203.245.108 is not responding. The following recipients did not receive this message: Please reply to postmas...@openvswitch.org if you feel this message to be in error. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Support BFD, CFM, carrier, and LACP for port liveness.
Sorry for my late reply, I am on a different timezone (Amsterdam time,
GMT+2).
It is good to bring back the amounts of operations in
odp_port_is_alive() as it is checked at every packet, hence a single
variable containing a summary of all combinations is the right way.
To answer your question, the patch works well. I toyed around with
min_rx and min_tx a bit and it also appears to change output ports fast
at first sight.
Best regards,
Niels
On do, 2014-10-16 at 15:29 -0700, Ben Pfaff wrote:
> On Thu, Oct 16, 2014 at 03:19:03PM -0700, Alex Wang wrote:
> > On Thu, Oct 16, 2014 at 3:00 PM, Ben Pfaff wrote:
> >
> > > This is simpler and shorter than handling each of these by itself.
> > >
> > > CC: Niels van Adrichem
> > > Suggested-by: Alex Wang
> > > Signed-off-by: Ben Pfaff
> > > ---
> > > ofproto/ofproto-dpif-xlate.c | 17 ++---
> > > 1 file changed, 2 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > > index 48576ad..d3ad62a 100644
> > > --- a/ofproto/ofproto-dpif-xlate.c
> > > +++ b/ofproto/ofproto-dpif-xlate.c
> > > @@ -1246,21 +1246,8 @@ ofp_port_to_odp_port(const struct xbridge *xbridge,
> > > ofp_port_t ofp_port)
> > > static bool
> > > odp_port_is_alive(const struct xlate_ctx *ctx, ofp_port_t ofp_port)
> > > {
> > > -struct xport *xport;
> > > -struct bfd *bfd;
> > > -
> > > -xport = get_ofp_port(ctx->xbridge, ofp_port);
> > > -if (!xport || xport->config & OFPUTIL_PC_PORT_DOWN ||
> > > -xport->state & OFPUTIL_PS_LINK_DOWN) {
> > > -return false;
> > > -}
> >
> >
> >
> > > -bfd = xport->bfd;
> > > -if (bfd && !bfd_forwarding(bfd)) {
> > > -return false;
> > > -}
> > > -
> > > -return true;
> > > +struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port);
> > > +return xport && xport->may_enable;
> > > }
> > >
> > > static struct ofputil_bucket *
> > > --
> > > 1.7.10.4
> > >
> > >
> > This looks very clean, port_run() does everything! including the
> > netdev_get_carrier() check~
> >
> >
> > Acked-by: Alex Wang
>
> Thanks. I decided to apply it right away because this looks like the
> right approach to me too.
>
> Niels, please do test it and let us know if it does not work for you.
>
> Thanks,
>
> Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Support BFD, CFM, carrier, and LACP for port liveness.
I'm happy to hear that it works for you. Thank you for testing!
On Fri, Oct 17, 2014 at 07:41:38AM +, Niels van Adrichem wrote:
> Sorry for my late reply, I am on a different timezone (Amsterdam time,
> GMT+2).
>
> It is good to bring back the amounts of operations in
> odp_port_is_alive() as it is checked at every packet, hence a single
> variable containing a summary of all combinations is the right way.
>
> To answer your question, the patch works well. I toyed around with
> min_rx and min_tx a bit and it also appears to change output ports fast
> at first sight.
>
> Best regards,
> Niels
>
>
> On do, 2014-10-16 at 15:29 -0700, Ben Pfaff wrote:
> > On Thu, Oct 16, 2014 at 03:19:03PM -0700, Alex Wang wrote:
> > > On Thu, Oct 16, 2014 at 3:00 PM, Ben Pfaff wrote:
> > >
> > > > This is simpler and shorter than handling each of these by itself.
> > > >
> > > > CC: Niels van Adrichem
> > > > Suggested-by: Alex Wang
> > > > Signed-off-by: Ben Pfaff
> > > > ---
> > > > ofproto/ofproto-dpif-xlate.c | 17 ++---
> > > > 1 file changed, 2 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > > > index 48576ad..d3ad62a 100644
> > > > --- a/ofproto/ofproto-dpif-xlate.c
> > > > +++ b/ofproto/ofproto-dpif-xlate.c
> > > > @@ -1246,21 +1246,8 @@ ofp_port_to_odp_port(const struct xbridge
> > > > *xbridge,
> > > > ofp_port_t ofp_port)
> > > > static bool
> > > > odp_port_is_alive(const struct xlate_ctx *ctx, ofp_port_t ofp_port)
> > > > {
> > > > -struct xport *xport;
> > > > -struct bfd *bfd;
> > > > -
> > > > -xport = get_ofp_port(ctx->xbridge, ofp_port);
> > > > -if (!xport || xport->config & OFPUTIL_PC_PORT_DOWN ||
> > > > -xport->state & OFPUTIL_PS_LINK_DOWN) {
> > > > -return false;
> > > > -}
> > >
> > >
> > >
> > > > -bfd = xport->bfd;
> > > > -if (bfd && !bfd_forwarding(bfd)) {
> > > > -return false;
> > > > -}
> > > > -
> > > > -return true;
> > > > +struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port);
> > > > +return xport && xport->may_enable;
> > > > }
> > > >
> > > > static struct ofputil_bucket *
> > > > --
> > > > 1.7.10.4
> > > >
> > > >
> > > This looks very clean, port_run() does everything! including the
> > > netdev_get_carrier() check~
> > >
> > >
> > > Acked-by: Alex Wang
> >
> > Thanks. I decided to apply it right away because this looks like the
> > right approach to me too.
> >
> > Niels, please do test it and let us know if it does not work for you.
> >
> > Thanks,
> >
> > Ben.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] rhel:Change service providers to 'openvswitch'
Thanks. I marked Dongdong as the author and retained his Signed-off-by, and changed your Signed-off-by to an Acked-by to indicate that you reviewed and approved. I applied this to master. Thanks again! On Fri, Oct 17, 2014 at 01:43:10AM +, Lichunhe wrote: > We work together, and talk about how to resolve this problem. Written > by dongdong, reviewed by me. > I generate the patch and send to you. The Author is Dongdong, you could > remove my signed off. > > Thanks. > > >-Original Message- > >From: Ben Pfaff [mailto:b...@nicira.com] > >Sent: Friday, October 17, 2014 2:50 AM > >To: Lichunhe > >Cc: dev@openvswitch.org; Liuyongan; dongdong (A); Qianhuibin > >Subject: Re: [ovs-dev] [PATCH 2/2] rhel:Change service providers to > >'openvswitch' > > > >On Wed, Oct 15, 2014 at 10:00:29AM +0800, lichu...@huawei.com wrote: > >> From: Chunhe Li > >> > >> In rethat OS, the service name is "openvswitch", providers also should > >> be change to "openvswitch" > >> > >> Signed-off-by: Chunhe Li > >> Signed-off-by: Dongdong > > > >The sign-off chain leaves me confused. The first sign-off should be the > >patch's > >author, which looks correct (if Chunhe Li is the author). > >Subsequent sign-offs should generally be by people who subsequently passed > >the patch along. Can you explain Dongdong's involvement? > > > >Thanks, > > > >Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/4] datapath-windows: event read should not fail when no events
Acked-by: Ankur Sharma
From: dev on behalf of Nithin Raju
Sent: Thursday, October 16, 2014 10:52 PM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH 1/4] datapath-windows: event read should not fail
when no events
The semantics are read operation are generally to return 0 bytes and
STATUS_SUCCESS when there are no events.
Also, added a fix to assign the PID to the synthetic OVS_MESSAGE formed
for the command validation.
Signed-off-by: Nithin Raju
---
datapath-windows/ovsext/Datapath.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/datapath-windows/ovsext/Datapath.c
b/datapath-windows/ovsext/Datapath.c
index 6cb9398..0d87a6d 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -728,6 +728,7 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
ovsMsg = &ovsMsgReadOp;
ovsMsg->nlMsg.nlmsgType = OVS_WIN_NL_CTRL_FAMILY_ID;
+ovsMsg->nlMsg.nlmsgPid = instance->pid;
/* An "artificial" command so we can use NL family function table*/
ovsMsg->genlMsg.cmd = (code == OVS_IOCTL_READ_EVENT) ?
OVS_CTRL_CMD_EVENT_NOTIFY :
@@ -2289,6 +2290,9 @@ OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT
usrParamsCtx,
/* remove an event entry from the event queue */
status = OvsRemoveEventEntry(usrParamsCtx->ovsInstance, &eventEntry);
if (status != STATUS_SUCCESS) {
+/* If there were not elements, read should return no data. */
+status = STATUS_SUCCESS;
+*replyLen = 0;
goto cleanup;
}
--
1.7.4.1
___
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=f6EhnZ0ORGZNt5QbYmRaOxfWfx%2Bqd3KEiPf3%2FYaollU%3D%0A&m=%2FM48eUyd5BYNGeIWYk67G8U7RLWOj6ZeioDQvBuNaaM%3D%0A&s=e084fcae0762e210e4e83ec84fd6525c6290b4bfbae4d92f7844f6ff532fce7e
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 3/4] datapath-windows: Fixes in packet created for userspace
Hi Nithin,
Changes look fine.
Please find a minor comment inline.
Acked-by: Ankur Sharma
From: dev on behalf of Nithin Raju
Sent: Thursday, October 16, 2014 10:52 PM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH 3/4] datapath-windows: Fixes in packet created for
userspace
A couple of miscellaneous fixes in code that creates a packet for
userspace as well as when we copy the packet to memory specified by
userspace.
Signed-off-by: Nithin Raju
---
datapath-windows/ovsext/User.c | 12 +---
1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c
index a8128bc..2811092 100644
--- a/datapath-windows/ovsext/User.c
+++ b/datapath-windows/ovsext/User.c
@@ -239,7 +239,7 @@ OvsReadDpIoctl(PFILE_OBJECT fileObject,
*ptr = sum;
ovsUserStats.l4Csum++;
} else {
-RtlCopyMemory(outputBuffer, &elem->packet, len);
+RtlCopyMemory(outputBuffer, &elem->packet.data, len);
}
*replyLen = len;
@@ -928,14 +928,14 @@ OvsCreateQueueNlPacket(PVOID userData,
UINT32 pid;
UINT32 nlMsgSize;
NL_BUFFER nlBuf;
+PNL_MSG_HDR nlMsg;
/* XXX pass vport in the stack rather than portNo */
POVS_VPORT_ENTRY vport =
OvsFindVportByPortNo(gOvsSwitchContext, inPort);
if (vport == NULL){
-/* Should never happen as dispatch lock is held */
-ASSERT(vport);
+/* No vport is not fatal. */
return NULL;
}
@@ -1064,6 +1064,12 @@ OvsCreateQueueNlPacket(PVOID userData,
elem->hdrInfo.l4Offset += VLAN_TAG_SIZE;
ovsUserStats.vlanInsert++;
}
+
+nlMsg = (PNL_MSG_HDR)NlBufAt(&nlBuf, 0, 0);
+nlMsg->nlmsgLen = NlBufSize(&nlBuf);
[ANKUR]: Should we not align the msgLen here.?
+/* 'totalLen' should be size of valid data. */
+elem->packet.totalLen = nlMsg->nlmsgLen;
+
return elem;
fail:
OvsFreeMemory(elem);
--
1.7.4.1
___
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=f6EhnZ0ORGZNt5QbYmRaOxfWfx%2Bqd3KEiPf3%2FYaollU%3D%0A&m=Hs49WkZV2%2BZ9pazMuLNebGUUSFx73gcCclLNMTE1CVc%3D%0A&s=428428febeae8b4f8477e5581fb55a2688c0ea9ecca16cbaa456cfa474d69940
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] xenserver: Add ovs-docker to the spec file.
On Thu, Oct 16, 2014 at 05:54:30PM -0700, Gurucharan Shetty wrote: > Fixes a rpmbuild failure. > > Signed-off-by: Gurucharan Shetty Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/4] datapath-windows: fixes in Flow.c in key parsing
Hi Nithin,
While reading the ETHERTYPE attribute we already use ntohs.
Regards,
Ankur
From: dev on behalf of Nithin Raju
Sent: Thursday, October 16, 2014 10:52 PM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH 2/4] datapath-windows: fixes in Flow.c in key parsing
Signed-off-by: Nithin Raju
---
datapath-windows/ovsext/Flow.c | 43 ---
1 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c
index f3ee726..fa61262 100644
--- a/datapath-windows/ovsext/Flow.c
+++ b/datapath-windows/ovsext/Flow.c
@@ -1267,20 +1267,19 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
* requests with no ETHERTYPE attributes.
* Need to verify this. */
if (keyAttrs[OVS_KEY_ATTR_ETHERTYPE]) {
-destKey->l2.dlType = ntohs((NlAttrGetU16(keyAttrs
- [OVS_KEY_ATTR_ETHERTYPE])));
+destKey->l2.dlType = ntohs((NlAttrGetU16(keyAttrs
+[OVS_KEY_ATTR_ETHERTYPE])));
}
if (keyAttrs[OVS_KEY_ATTR_VLAN]) {
-destKey->l2.vlanTci = NlAttrGetU16(keyAttrs
- [OVS_KEY_ATTR_VLAN]);
+destKey->l2.vlanTci = NlAttrGetU16(keyAttrs[OVS_KEY_ATTR_VLAN]);
}
/* L3 + L4. */
destKey->l2.keyLen = OVS_WIN_TUNNEL_KEY_SIZE + OVS_L2_KEY_SIZE
- destKey->l2.offset;
-switch (destKey->l2.dlType) {
+switch (ntohs(destKey->l2.dlType)) {
case ETH_TYPE_IPV4: {
if (keyAttrs[OVS_KEY_ATTR_IPV4]) {
@@ -1395,22 +1394,24 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
}
case ETH_TYPE_ARP:
case ETH_TYPE_RARP: {
-ArpKey *arpFlowPutKey = &destKey->arpKey;
-const struct ovs_key_arp *arpKey;
-
-arpKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_ARP]);
-
-arpFlowPutKey->nwSrc = arpKey->arp_sip;
-arpFlowPutKey->nwDst = arpKey->arp_tip;
-
-RtlCopyMemory(arpFlowPutKey->arpSha, arpKey->arp_sha, ETH_ADDR_LEN);
-RtlCopyMemory(arpFlowPutKey->arpTha, arpKey->arp_tha, ETH_ADDR_LEN);
-arpFlowPutKey->nwProto = (UINT8)(arpKey->arp_op);
-arpFlowPutKey->pad[0] = 0;
-arpFlowPutKey->pad[1] = 0;
-arpFlowPutKey->pad[2] = 0;
-destKey->l2.keyLen += OVS_ARP_KEY_SIZE;
-break;
+if (keyAttrs[OVS_KEY_ATTR_ARP]) {
+ArpKey *arpFlowPutKey = &destKey->arpKey;
+const struct ovs_key_arp *arpKey;
+
+arpKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_ARP]);
+
+arpFlowPutKey->nwSrc = arpKey->arp_sip;
+arpFlowPutKey->nwDst = arpKey->arp_tip;
+
+RtlCopyMemory(arpFlowPutKey->arpSha, arpKey->arp_sha,
ETH_ADDR_LEN);
+RtlCopyMemory(arpFlowPutKey->arpTha, arpKey->arp_tha,
ETH_ADDR_LEN);
+arpFlowPutKey->nwProto = (UINT8)(arpKey->arp_op);
+arpFlowPutKey->pad[0] = 0;
+arpFlowPutKey->pad[1] = 0;
+arpFlowPutKey->pad[2] = 0;
+destKey->l2.keyLen += OVS_ARP_KEY_SIZE;
+break;
+}
}
}
}
--
1.7.4.1
___
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=f6EhnZ0ORGZNt5QbYmRaOxfWfx%2Bqd3KEiPf3%2FYaollU%3D%0A&m=jCDA1pWsDkBchl%2B6LQPbLbMuTXTPlVqZIjhz2oFAtYI%3D%0A&s=a2a2699705f1dd5d7c08c077b9373536039ccff323849677705d82283800ce3b
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 4/4] lib/netlink-socket.c: fixes in nl_sock_recv__() on Windows
Hi Nithin,
Please find my comments inline.
Regards,
Ankur
From: dev on behalf of Nithin Raju
Sent: Thursday, October 16, 2014 10:52 PM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH 4/4] lib/netlink-socket.c: fixes in
nl_sock_recv__() on Windows
In nl_sock_recv__() on Windows, we realloc a new ofpbuf to copy received
data if the caller specified buffer is small. While we do so, we need
reset some of the other stack variables to point to the new ofpbuf.
Other fixes are around using 'error' rather than 'errno'.
Acked-by: Nithin Raju
---
lib/netlink-socket.c | 10 ++
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
index 7e30ab1..68e81d1 100644
--- a/lib/netlink-socket.c
+++ b/lib/netlink-socket.c
@@ -382,8 +382,8 @@ nl_sock_join_mcgroup(struct nl_sock *sock, unsigned int
multicast_group)
if (error) {
sock->read_ioctl = OVS_IOCTL_READ;
VLOG_WARN("could not join multicast group %u (%s)",
- multicast_group, ovs_strerror(errno));
-return errno;
+ multicast_group, ovs_strerror(error));
+return error;
}
#else
if (setsockopt(sock->fd, SOL_NETLINK, NETLINK_ADD_MEMBERSHIP,
@@ -413,8 +413,8 @@ nl_sock_leave_mcgroup(struct nl_sock *sock, unsigned int
multicast_group)
int error = nl_sock_mcgroup(sock, multicast_group, false);
if (error) {
VLOG_WARN("could not leave multicast group %u (%s)",
- multicast_group, ovs_strerror(errno));
-return errno;
+ multicast_group, ovs_strerror(error));
+return error;
}
sock->read_ioctl = OVS_IOCTL_READ;
#else
@@ -548,6 +548,8 @@ nl_sock_recv__(struct nl_sock *sock, struct ofpbuf *buf,
bool wait)
} else {
if (retval >= buf->allocated) {
ofpbuf_reinit(buf, retval);
+nlmsghdr = ofpbuf_base(buf);
+nlmsghdr->nlmsg_len = UINT32_MAX;
[ANKUR]: Why are we assigning nlmsg_len to UINT32_MAX and not retval (aligned).
}
memcpy(ofpbuf_data(buf), tail, retval);
ofpbuf_set_size(buf, retval);
--
1.7.4.1
___
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=f6EhnZ0ORGZNt5QbYmRaOxfWfx%2Bqd3KEiPf3%2FYaollU%3D%0A&m=f9MF4SPniXIonGt8lRQItVrFGQnWeNxZsT45uXpz2X4%3D%0A&s=a7066a9f8390191d61e0f73f7be911a7a0e900f0276ed908a26d56739a39a388
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Add OpenFlow support for 802.1AD Ethertype
Ben,
Thanks for your thoughtful comments. Apologies for my late reply...
Dave.
On Fri, Oct 10, 2014 at 6:43 PM, Ben Pfaff wrote:
> On Fri, Oct 10, 2014 at 10:06:29AM -0400, Dave Benson wrote:
> > Previous behavior forced the use of the 802.1Q Ethertype for all VLAN
> headers.
> > In order to open the door for future support of VLAN stacking or
> Provider Based
> > Bridging, support of the 802.1AD Ethertype is a logical first step.
> >
> > This patch adds support for OpenFlow flows to specify push_vlan actions
> which
> > may now specify the 0x88a8 Ethertype. Userspace and fastpath forwarding
> are
> > supported. The various mechanisms to dump flows correctly show the two
> > possible Ethertypes.
> >
> > Bridging (aka normal mode) has not been addressed with this patch.
> >
> > Testing:
> > 1 Added a test to ofproto-dpif.at.
> > 2 Tested on Xubuntu 14.04 (kernel version 3.13). Created two OVS
> bridges with
> > a physical ethernet between them. Attached VM clients and passed ping
> and
> > iperf traffic between the VMs. Tested configurations using both
> possible
> > VLAN Ethertypes. Verified correct packet formats with wireshark.
> > Verified the various dump-flow commands (ofctl, dpif and dpctl)
> returned the
> > correct information.
> > 3 Prior to the rebase to latest, testing was also done on Xubuntu 13.10
> (kernel
> > version 3.11).
> >
> > This work was done independantly, although I read with interest the
> patches from
> > Thomas Herbert and Avinash Andrew and the related comments. I believe
> this work
> > will be complementary to further efforts in this area.
>
> Thank you for your contribution. I have some comments.
>
> One direction that we will probably have to take at some point is to
> allow matching and setting the DEI bit. Until now this has been
> somewhat difficult because of the special use we made of that bit (to
> indicate that a VLAN was present). With a separate TPID member in the
> flow, we can move away from that, making it easier to add DEI support.
>
Agreed. I considered making that change for VLAN presence, but thought my
first patch shouldn't over-reach.
I think that match_format() should include the tpid when it is
> relevant.
>
> Certainly, but we can't yet match on the tpid...
With this change, I think that an OpenFlow flow table entry that
> matches on a VLAN will now match both 0x88a8 and 0x8100 tags for that
> VLAN. I doubt that that is desirable behavior, because my
> understanding is that 0x88a8 and 0x8100 tags are generally in
> different namespaces and are not interchangeable.
Definately Agree. I saw that the OF spec has the same problem. They
define the eth_type as 'Ethernet type of the OpenFlow packet payload, after
VLAN tags.' So there is nothing currently to match on VLAN tpid. Does OVS
go it's own way in a case like this, or discuss this with the OF crowd?
> We also need to
> decide how to handle 0x88a8 tags in the OpenFlow normal action (I see
> that you made a change in xlate_normal() in ofproto-dpif-xlate.c
> already, but it does not seem to address that issue).
>
> No, I did not address normal action.
> In flow_get_vlan_tpid(), this:
> if (!flow->vlan_tci & htons(VLAN_CFI)) {
> should likely be:
> if (!(flow->vlan_tci & htons(VLAN_CFI))) {
>
> Sure.
> Is flow_get_vlan_tpid() useful? That is, is there a way for an
> invalid tpid value to get into a flow's vlan_tpid member?
>
> I used a fn so I could provide a safe value if it wasn't set properly and
had debug logs in there originally. I was concerned that I could
adequately test all cases. For example, older OF used to simply do
mod_vlan_vid and not do a push_vlan.
> odp_execute_actions() should use the TPID provided in the push_vlan
> action, e.g. 'vlan->vlan_tpid'.
>
> Yes.
> In odp_flow_key_from_flow__(), I imagine that the code here should use
> the VLAN type from the flow instead:
> /* If flow->dl_type is not already VLAN, then use typical VLAN
> eth type */
> if (!eth_type_vlan(flow->dl_type)) {
> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE,
> htons(ETH_TYPE_VLAN));
> } else {
> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE,
> flow->dl_type);
> }
> which would look something like:
> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE,
> flow->vlan_tpid ? flow->vlan_tpid : flow->dl_type);
>
> Yes.
> I think that commit_vlan_action() overlooks the possibility that the
> TCI stays the same but the TPID changes. To handle that, I think that
> you need to pop off the old VLAN header and then push on a new one.
>
> For compatibility with old datapaths, userspace needs to be able to
> detect whether the datapath supports non-0x8100 VLANs and fall back to
> userspace processing for them if it doesn't.
>
> Hadn't considered that datapath mismatch could occur. I'll look into it.
> The new struct ofpact_push is used only for pushing VLANs. I don't
Re: [ovs-dev] [PATCH] datapath: Rename last_action() as nla_is_last() and move to netlink.h
On Fri, Oct 17, 2014 at 12:16 AM, Simon Horman
wrote:
> On Thu, Oct 16, 2014 at 04:50:10PM -0700, Andy Zhou wrote:
>> Simon, The change makes a lot of sense.
>>
>> I am just wondering if we should upstream the netlink.h change first?
>> To me, it seems to add a new compat API that does not exist upstream.
>
> Sure, I think that makes sense. Though I'm not sure if upstream likes
> to take new API calls that aren't used. Perhaps a good way forwards
> would be for me to re-submit this patch against the upstream net-next
> kernel.
>
> Pravin, how would you feel about that?
>
Yes, lets submit it upstream and then backport it.
>>
>>
>> On Wed, Sep 24, 2014 at 9:28 PM, Simon Horman
>> wrote:
>> > The original motivation for this change was to allow the
>> > helper to be used in files other than actions.c as part
>> > of work on an odp select group action.
>> >
>> > It was as pointed out by Thomas Graf that this helper would be best
>> > off living in netlink.h. Furthermore, I think that the generic nature of
>> > this
>> > helper means it is best off in netlink.h regardless of if it is used more
>> > than
>> > one .c file or not. Thus I would like it considered independent
>> > of the work on an odp select group action.
>> >
>> > Signed-off-by: Simon Horman
>> > ---
>> > datapath/actions.c | 11 +++
>> > datapath/linux/compat/include/net/netlink.h | 4
>> > 2 files changed, 7 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/datapath/actions.c b/datapath/actions.c
>> > index b527cb6..535b87e 100644
>> > --- a/datapath/actions.c
>> > +++ b/datapath/actions.c
>> > @@ -671,11 +671,6 @@ static int output_userspace(struct datapath *dp,
>> > struct sk_buff *skb,
>> > return ovs_dp_upcall(dp, skb, key, &upcall);
>> > }
>> >
>> > -static bool last_action(const struct nlattr *a, int rem)
>> > -{
>> > - return a->nla_len == rem;
>> > -}
>> > -
>> > static int sample(struct datapath *dp, struct sk_buff *skb,
>> > struct sw_flow_key *key, const struct nlattr *attr)
>> > {
>> > @@ -710,7 +705,7 @@ static int sample(struct datapath *dp, struct sk_buff
>> > *skb,
>> > * user space. This skb will be consumed by its caller.
>> > */
>> > if (likely(nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
>> > - last_action(a, rem)))
>> > + nla_is_last(a, rem)))
>> > return output_userspace(dp, skb, key, a);
>> >
>> > skb = skb_clone(skb, GFP_ATOMIC);
>> > @@ -810,7 +805,7 @@ static int execute_recirc(struct datapath *dp, struct
>> > sk_buff *skb,
>> > }
>> > BUG_ON(!is_flow_key_valid(key));
>> >
>> > - if (!last_action(a, rem)) {
>> > + if (!nla_is_last(a, rem)) {
>> > /* Recirc action is the not the last action
>> > * of the action list, need to clone the skb.
>> > */
>> > @@ -897,7 +892,7 @@ static int do_execute_actions(struct datapath *dp,
>> > struct sk_buff *skb,
>> >
>> > case OVS_ACTION_ATTR_RECIRC:
>> > err = execute_recirc(dp, skb, key, a, rem);
>> > - if (last_action(a, rem)) {
>> > + if (nla_is_last(a, rem)) {
>> > /* If this is the last action, the skb has
>> > * been consumed or freed.
>> > * Return immediately.
>> > diff --git a/datapath/linux/compat/include/net/netlink.h
>> > b/datapath/linux/compat/include/net/netlink.h
>> > index a6dc584..de37058 100644
>> > --- a/datapath/linux/compat/include/net/netlink.h
>> > +++ b/datapath/linux/compat/include/net/netlink.h
>> > @@ -63,4 +63,8 @@ static inline struct nlattr *nla_find_nested(struct
>> > nlattr *nla, int attrtype)
>> > }
>> > #endif
>> >
>> > +static inline bool nla_is_last(const struct nlattr *a, int rem)
>> > +{
>> > + return a->nla_len == rem;
>> > +}
>> > #endif /* net/netlink.h */
>> > --
>> > 2.0.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] lib/classifier: Make classifier iteration more robust.
I’ll see if we can make the iteration both safe and lockless, so there is no
need to review this patch at this moment.
Jarno
On Oct 16, 2014, at 4:24 PM, Jarno Rajahalme wrote:
> This patch changes the classifier internal mutex to a recursive type.
> This allows simplification of locking during iteration.
>
> Accurate iteration requires classifier_inserts to be exluded during
> iteration. To this end we take the internal mutex at the start of the
> iteration. To allow rule removal while iterating, we have supported a
> 'SAFE' variant of the iterator, that used to release the mutex for the
> body of the iterator loop. For a classifier user without additional
> locks this would have opened a possibility for another thread to issue
> classifier_insert()s during the iteration, potentially making the
> iteration to skip or duplicate entries due to cmap internal
> rearrangements.
>
> This patch fixes the above keeping the writers excluded also in the
> safe mode, which is possible with a recursive mutex. We also remove
> the distinction between 'SAFE' and normal iteration, as the only
> remaining difference was in the iterator for statement, and we can use
> the 'SAFE' variant in all cases without additional overhead.
>
> Signed-off-by: Jarno Rajahalme
> ---
> lib/classifier.c| 16 ++--
> lib/classifier.h| 33 ++---
> ofproto/ofproto-dpif.c |2 +-
> ofproto/ofproto.c |2 +-
> tests/test-classifier.c |5 ++---
> utilities/ovs-ofctl.c |2 +-
> 6 files changed, 25 insertions(+), 35 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index f30ba95..19a3435 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -448,7 +448,7 @@ void
> classifier_init(struct classifier *cls, const uint8_t *flow_segments)
> OVS_EXCLUDED(cls->mutex)
> {
> -ovs_mutex_init(&cls->mutex);
> +ovs_mutex_init_recursive(&cls->mutex);
> ovs_mutex_lock(&cls->mutex);
> cls->n_rules = 0;
> cmap_init(&cls->subtables_map);
> @@ -1255,14 +1255,12 @@ search_subtable(const struct cls_subtable *subtable,
> *
> * Ignores target->priority. */
> struct cls_cursor cls_cursor_start(const struct classifier *cls,
> - const struct cls_rule *target,
> - bool safe)
> + const struct cls_rule *target)
> OVS_NO_THREAD_SAFETY_ANALYSIS
> {
> struct cls_cursor cursor;
> struct cls_subtable *subtable;
>
> -cursor.safe = safe;
> cursor.cls = cls;
> cursor.target = target && !cls_rule_is_catchall(target) ? target : NULL;
> cursor.rule = NULL;
> @@ -1280,8 +1278,8 @@ struct cls_cursor cls_cursor_start(const struct
> classifier *cls,
> }
> }
>
> -/* Leave locked if requested and have a rule. */
> -if (safe || !cursor.rule) {
> +/* Release the lock if iteration ended already. */
> +if (!cursor.rule) {
> ovs_mutex_unlock(&cursor.cls->mutex);
> }
> return cursor;
> @@ -1328,11 +1326,9 @@ void
> cls_cursor_advance(struct cls_cursor *cursor)
> OVS_NO_THREAD_SAFETY_ANALYSIS
> {
> -if (cursor->safe) {
> -ovs_mutex_lock(&cursor->cls->mutex);
> -}
> cursor->rule = cls_cursor_next(cursor);
> -if (cursor->safe || !cursor->rule) {
> +/* Release the lock if iteration ended. */
> +if (!cursor->rule) {
> ovs_mutex_unlock(&cursor->cls->mutex);
> }
> }
> diff --git a/lib/classifier.h b/lib/classifier.h
> index d1f4e86..0d6ee2c 100644
> --- a/lib/classifier.h
> +++ b/lib/classifier.h
> @@ -319,39 +319,34 @@ struct cls_cursor {
> struct cmap_cursor subtables;
> struct cmap_cursor rules;
> struct cls_rule *rule;
> -bool safe;
> };
>
> -/* Iteration requires mutual exclusion of writers. We do this by taking
> - * a mutex for the duration of the iteration, except for the
> - * 'SAFE' variant, where we release the mutex for the body of the loop. */
> +/* Accurate iteration requires mutual exclusion of writers. We do this by
> + * taking a classifier internal mutex for the duration of the iteration.
> This
> + * implies that the iteration MUST NOT be terminated by jumping out of the
> loop
> + * with break or return statements, lest the classifier be left locked, and
> + * other threads deadlock.
> + *
> + * The iterating thread may modify the classifier itself. However,
> + * classifier_insert()s can rearrange the internal structures so that the
> + * iteration can go through same rules twice or miss some rules altogether.
> + * classifier_replace() and classifier_remove() do not cause similar
> + * problems. */
> struct cls_cursor cls_cursor_start(const struct classifier *cls,
> - const struct cls_rule *target,
> - bool safe);
> + const struct cls_rule *target);
>
> void cls_cursor_advance(struct cls_cursor *);
[ovs-dev] [PATCH v5] datapath-windows: netdev-windows fixes for vswitchd bringup.
Added the handlers for update_flags and set_etheraddr.
These handlers were needed for vswitchd bringup on windows
platform.
Signed-off-by: Ankur Sharma
Acked-by: Nithin Raju
---
lib/netdev-windows.c | 30 +-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/lib/netdev-windows.c b/lib/netdev-windows.c
index 02d37a7..f9f96ec 100644
--- a/lib/netdev-windows.c
+++ b/lib/netdev-windows.c
@@ -326,7 +326,33 @@ netdev_windows_get_mtu(const struct netdev *netdev_, int
*mtup)
}
return 0;
}
-
+
+/* This functionality is not really required by the datapath.
+ * But vswitchd bringup expects this to be implemented. */
+static int
+netdev_windows_set_etheraddr(const struct netdev *netdev_, uint8_t mac[6])
+{
+return 0;
+}
+
+/* We do not really have to update anything in kernel. */
+static int
+netdev_win_set_flag(const char *name, uint32_t flags)
+{
+return 0;
+}
+
+/* This functionality is not really required by the datapath.
+ * But vswitchd bringup expects this to be implemented. */
+static int
+netdev_win_update_flags_system(struct netdev *netdev_,
+ enum netdev_flags off,
+ enum netdev_flags on,
+ enum netdev_flags *old_flagsp)
+{
+return 0;
+}
+
static int
netdev_windows_internal_construct(struct netdev *netdev_)
@@ -343,6 +369,8 @@ netdev_windows_internal_construct(struct netdev *netdev_)
.destruct = netdev_windows_destruct, \
.dealloc= netdev_windows_dealloc, \
.get_etheraddr = netdev_windows_get_etheraddr, \
+.set_etheraddr = netdev_windows_set_etheraddr, \
+.update_flags = netdev_win_update_flags_system, \
}
const struct netdev_class netdev_windows_class =
--
1.9.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] stream-tcp: Call setsockopt TCP_NODELAY after TCP is connected for Windows.
On Tue, Oct 7, 2014 at 2:25 PM, Ben Pfaff wrote: > On Thu, Oct 02, 2014 at 08:16:56AM -0700, Gurucharan Shetty wrote: >> On Windows platform, TCP_NODELAY can only be set when TCP is established. >> The current code does not create any problems while running unit tests >> (because connections get established immediately) but is observed while >> connecting to a different machine. >> >> commit 8b76839(Move setsockopt TCP_NODELAY to when TCP is connected.) >> made changes to call setsockopt with TCP_NODELAY after TCP is connected >> only in lib/stream-ssl.c. We need the same change for stream-tcp too. >> >> Signed-off-by: Gurucharan Shetty > > I guess we will need a similar change for stream-ssl.c. As noted in the commit message, stream-ssl already has the change. stream-tcp was missing it. > > I'm not a fan of unnecessary platform-related differences. That makes > me think that it might be better to set TCP_NODELAY only after > connecting on every platform. After all, there is no benefit, except > simplicity, to setting it before one can actually send any data, and > we cannot get the simplicity benefit anyway. > > The one complication is that on Windows, stream-fd is only used for > TCP sockets, whereas on Unix it is used for Unix domain sockets and > TCP sockets, and one cannot set TCP_NODELAY on a non-TCP socket. I > guess that means that we'd need a new parameter to new_fd_stream() to > indicate whether it is a TCP socket. That's probably OK though. > > Looking at a diff, the differences between the two versions of > stream-fd are not large, and some of the differences are unnecessary. > Maybe this change is an argument toward re-unification. I agree. As a first step, I will send in a patch that merges stream-fd-windows and stream-fd-unix. Once that looks fine, I will resping this patch. Thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] stream-fd: Merge stream-fd-windows and stream-fd-unix.
There was not much difference between the two files after moving
all of the Windows socket HANDLE polling functionality to poll-loop.c.
So merge them together.
Signed-off-by: Gurucharan Shetty
---
lib/automake.mk |5 +-
lib/stream-fd-windows.c | 270 -
lib/{stream-fd-unix.c => stream-fd.c} | 55 +--
3 files changed, 44 insertions(+), 286 deletions(-)
delete mode 100644 lib/stream-fd-windows.c
rename lib/{stream-fd-unix.c => stream-fd.c} (86%)
diff --git a/lib/automake.mk b/lib/automake.mk
index ec4ad8e..b618f8f 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -212,6 +212,7 @@ lib_libopenvswitch_la_SOURCES = \
lib/sset.h \
lib/stp.c \
lib/stp.h \
+ lib/stream-fd.c \
lib/stream-fd.h \
lib/stream-provider.h \
lib/stream-ssl.h \
@@ -265,8 +266,7 @@ lib_libopenvswitch_la_SOURCES += \
lib/getrusage-windows.c \
lib/latch-windows.c \
lib/route-table-stub.c \
- lib/strsep.c \
- lib/stream-fd-windows.c
+ lib/strsep.c
else
lib_libopenvswitch_la_SOURCES += \
lib/daemon-unix.c \
@@ -274,7 +274,6 @@ lib_libopenvswitch_la_SOURCES += \
lib/signals.c \
lib/signals.h \
lib/socket-util-unix.c \
- lib/stream-fd-unix.c \
lib/stream-unix.c
endif
diff --git a/lib/stream-fd-windows.c b/lib/stream-fd-windows.c
deleted file mode 100644
index e366c71..000
--- a/lib/stream-fd-windows.c
+++ /dev/null
@@ -1,270 +0,0 @@
-/*
- * Copyright (c) 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.
- * You may obtain a copy of the License at:
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#include
-#include "stream-fd.h"
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include "fatal-signal.h"
-#include "poll-loop.h"
-#include "socket-util.h"
-#include "stream.h"
-#include "stream-provider.h"
-#include "util.h"
-#include "vlog.h"
-
-VLOG_DEFINE_THIS_MODULE(stream_fd_windows);
-
-/* Active file descriptor stream. */
-
-struct stream_fd
-{
-struct stream stream;
-int fd;
-};
-
-static const struct stream_class stream_fd_class;
-
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(10, 25);
-
-/* Creates a new stream named 'name' that will send and receive data on 'fd'
- * and stores a pointer to the stream in '*streamp'. Initial connection status
- * 'connect_status' is interpreted as described for stream_init().
- *
- * Returns 0 if successful, otherwise a positive errno value. (The current
- * implementation never fails.) */
-int
-new_fd_stream(const char *name, int fd, int connect_status,
- struct stream **streamp)
-{
-struct stream_fd *s;
-
-s = xmalloc(sizeof *s);
-stream_init(&s->stream, &stream_fd_class, connect_status, name);
-s->fd = fd;
-*streamp = &s->stream;
-return 0;
-}
-
-static struct stream_fd *
-stream_fd_cast(struct stream *stream)
-{
-stream_assert_class(stream, &stream_fd_class);
-return CONTAINER_OF(stream, struct stream_fd, stream);
-}
-
-static void
-fd_close(struct stream *stream)
-{
-struct stream_fd *s = stream_fd_cast(stream);
-closesocket(s->fd);
-free(s);
-}
-
-static int
-fd_connect(struct stream *stream)
-{
-struct stream_fd *s = stream_fd_cast(stream);
-return check_connection_completion(s->fd);
-}
-
-static ssize_t
-fd_recv(struct stream *stream, void *buffer, size_t n)
-{
-struct stream_fd *s = stream_fd_cast(stream);
-ssize_t retval;
-
-retval = recv(s->fd, buffer, n, 0);
-if (retval < 0) {
-retval = -sock_errno();
-}
-if (retval == -WSAEWOULDBLOCK) {
-return -EAGAIN;
-}
-return retval;
-}
-
-static ssize_t
-fd_send(struct stream *stream, const void *buffer, size_t n)
-{
-struct stream_fd *s = stream_fd_cast(stream);
-ssize_t retval;
-
-retval = send(s->fd, buffer, n, 0);
-if (retval < 0) {
-retval = -sock_errno();
-}
-if (retval == -WSAEWOULDBLOCK) {
-return -EAGAIN;
-}
-
-return retval;
-}
-
-static void
-fd_wait(struct stream *stream, enum stream_wait_type wait)
-{
-struct stream_fd *s = stream_fd_cast(stream);
-switch (wait) {
-case STREAM_CONNECT:
-case STREAM_SEND:
-poll_fd_wait(s->fd, POLLOUT);
-break;
-
-case STREAM_RECV:
-poll_fd_wait(s->fd, POLLIN);
-break;
-
-default:
-OVS_NOT_REACHED();
-}
-}
-
-static const struct stream_class stream_fd_class = {
Re: [ovs-dev] [PATCH] datapath: Rename last_action() as nla_is_last() and move to netlink.h
2014/10/17 19:25 "Pravin Shelar" :
>
> On Fri, Oct 17, 2014 at 12:16 AM, Simon Horman
> wrote:
> > On Thu, Oct 16, 2014 at 04:50:10PM -0700, Andy Zhou wrote:
> >> Simon, The change makes a lot of sense.
> >>
> >> I am just wondering if we should upstream the netlink.h change first?
> >> To me, it seems to add a new compat API that does not exist upstream.
> >
> > Sure, I think that makes sense. Though I'm not sure if upstream likes
> > to take new API calls that aren't used. Perhaps a good way forwards
> > would be for me to re-submit this patch against the upstream net-next
> > kernel.
> >
> > Pravin, how would you feel about that?
> >
> Yes, lets submit it upstream and then backport it.
Thanks, I'll cook up a patch for upstream.
> >> On Wed, Sep 24, 2014 at 9:28 PM, Simon Horman
> >> wrote:
> >> > The original motivation for this change was to allow the
> >> > helper to be used in files other than actions.c as part
> >> > of work on an odp select group action.
> >> >
> >> > It was as pointed out by Thomas Graf that this helper would be best
> >> > off living in netlink.h. Furthermore, I think that the generic
nature of this
> >> > helper means it is best off in netlink.h regardless of if it is used
more than
> >> > one .c file or not. Thus I would like it considered independent
> >> > of the work on an odp select group action.
> >> >
> >> > Signed-off-by: Simon Horman
> >> > ---
> >> > datapath/actions.c | 11 +++
> >> > datapath/linux/compat/include/net/netlink.h | 4
> >> > 2 files changed, 7 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git a/datapath/actions.c b/datapath/actions.c
> >> > index b527cb6..535b87e 100644
> >> > --- a/datapath/actions.c
> >> > +++ b/datapath/actions.c
> >> > @@ -671,11 +671,6 @@ static int output_userspace(struct datapath
*dp, struct sk_buff *skb,
> >> > return ovs_dp_upcall(dp, skb, key, &upcall);
> >> > }
> >> >
> >> > -static bool last_action(const struct nlattr *a, int rem)
> >> > -{
> >> > - return a->nla_len == rem;
> >> > -}
> >> > -
> >> > static int sample(struct datapath *dp, struct sk_buff *skb,
> >> > struct sw_flow_key *key, const struct nlattr *attr)
> >> > {
> >> > @@ -710,7 +705,7 @@ static int sample(struct datapath *dp, struct
sk_buff *skb,
> >> > * user space. This skb will be consumed by its caller.
> >> > */
> >> > if (likely(nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
> >> > - last_action(a, rem)))
> >> > + nla_is_last(a, rem)))
> >> > return output_userspace(dp, skb, key, a);
> >> >
> >> > skb = skb_clone(skb, GFP_ATOMIC);
> >> > @@ -810,7 +805,7 @@ static int execute_recirc(struct datapath *dp,
struct sk_buff *skb,
> >> > }
> >> > BUG_ON(!is_flow_key_valid(key));
> >> >
> >> > - if (!last_action(a, rem)) {
> >> > + if (!nla_is_last(a, rem)) {
> >> > /* Recirc action is the not the last action
> >> > * of the action list, need to clone the skb.
> >> > */
> >> > @@ -897,7 +892,7 @@ static int do_execute_actions(struct datapath
*dp, struct sk_buff *skb,
> >> >
> >> > case OVS_ACTION_ATTR_RECIRC:
> >> > err = execute_recirc(dp, skb, key, a, rem);
> >> > - if (last_action(a, rem)) {
> >> > + if (nla_is_last(a, rem)) {
> >> > /* If this is the last action, the
skb has
> >> > * been consumed or freed.
> >> > * Return immediately.
> >> > diff --git a/datapath/linux/compat/include/net/netlink.h
b/datapath/linux/compat/include/net/netlink.h
> >> > index a6dc584..de37058 100644
> >> > --- a/datapath/linux/compat/include/net/netlink.h
> >> > +++ b/datapath/linux/compat/include/net/netlink.h
> >> > @@ -63,4 +63,8 @@ static inline struct nlattr
*nla_find_nested(struct nlattr *nla, int attrtype)
> >> > }
> >> > #endif
> >> >
> >> > +static inline bool nla_is_last(const struct nlattr *a, int rem)
> >> > +{
> >> > + return a->nla_len == rem;
> >> > +}
> >> > #endif /* net/netlink.h */
> >> > --
> >> > 2.0.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 v3] lib/dpif-netdev: Integrate megaflow classifier.
On Oct 16, 2014, at 11:09 PM, Alex Wang wrote:
> Thx for the revision,
>
>
> +/* Removes 'rule' from 'cls', also destructing the 'rule'. */
> +static void
> +dpcls_remove(struct dpcls *cls, struct dpcls_rule *rule)
> +{
> +struct dpcls_subtable *subtable;
> +
> +ovs_assert(rule->mask);
> +
> +INIT_CONTAINER(subtable, rule->mask, mask);
> +
> +if (cmap_remove(&subtable->rules, &rule->cmap_node, rule->flow.hash)
> +== 0) {
> +dpcls_destroy_subtable(cls, subtable);
> +}
> +
> +rule->mask = NULL;
> +}
>
>
> The assignment here caused a segfault in test, since there could be pmd
> threads referring to the flow when dpcls_remove() is called. We should not
> reset the pointer.
>
Thank you for testing and debugging!
>
> 32 UDP flows Spirent test showed no observable improvement but let's
> wait for the per-pmd-thread classifier/flow-table~
>
This was expected, any improvement should be visible only when there is high
churn on the megaflow cache.
> Otherwise, all tested and looked good~
> Acked-by: Alex Wang
>
Pushed to master with the fix,
Jarno
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Rename last_action() as nla_is_last() and move to netlink.h
Thanks.
On Fri, Oct 17, 2014 at 1:24 PM, Simon Horman
wrote:
>
> 2014/10/17 19:25 "Pravin Shelar" :
>>
>> On Fri, Oct 17, 2014 at 12:16 AM, Simon Horman
>> wrote:
>> > On Thu, Oct 16, 2014 at 04:50:10PM -0700, Andy Zhou wrote:
>> >> Simon, The change makes a lot of sense.
>> >>
>> >> I am just wondering if we should upstream the netlink.h change first?
>> >> To me, it seems to add a new compat API that does not exist upstream.
>> >
>> > Sure, I think that makes sense. Though I'm not sure if upstream likes
>> > to take new API calls that aren't used. Perhaps a good way forwards
>> > would be for me to re-submit this patch against the upstream net-next
>> > kernel.
>> >
>> > Pravin, how would you feel about that?
>> >
>> Yes, lets submit it upstream and then backport it.
>
> Thanks, I'll cook up a patch for upstream.
>
>> >> On Wed, Sep 24, 2014 at 9:28 PM, Simon Horman
>> >> wrote:
>> >> > The original motivation for this change was to allow the
>> >> > helper to be used in files other than actions.c as part
>> >> > of work on an odp select group action.
>> >> >
>> >> > It was as pointed out by Thomas Graf that this helper would be best
>> >> > off living in netlink.h. Furthermore, I think that the generic nature
>> >> > of this
>> >> > helper means it is best off in netlink.h regardless of if it is used
>> >> > more than
>> >> > one .c file or not. Thus I would like it considered independent
>> >> > of the work on an odp select group action.
>> >> >
>> >> > Signed-off-by: Simon Horman
>> >> > ---
>> >> > datapath/actions.c | 11 +++
>> >> > datapath/linux/compat/include/net/netlink.h | 4
>> >> > 2 files changed, 7 insertions(+), 8 deletions(-)
>> >> >
>> >> > diff --git a/datapath/actions.c b/datapath/actions.c
>> >> > index b527cb6..535b87e 100644
>> >> > --- a/datapath/actions.c
>> >> > +++ b/datapath/actions.c
>> >> > @@ -671,11 +671,6 @@ static int output_userspace(struct datapath *dp,
>> >> > struct sk_buff *skb,
>> >> > return ovs_dp_upcall(dp, skb, key, &upcall);
>> >> > }
>> >> >
>> >> > -static bool last_action(const struct nlattr *a, int rem)
>> >> > -{
>> >> > - return a->nla_len == rem;
>> >> > -}
>> >> > -
>> >> > static int sample(struct datapath *dp, struct sk_buff *skb,
>> >> > struct sw_flow_key *key, const struct nlattr *attr)
>> >> > {
>> >> > @@ -710,7 +705,7 @@ static int sample(struct datapath *dp, struct
>> >> > sk_buff *skb,
>> >> > * user space. This skb will be consumed by its caller.
>> >> > */
>> >> > if (likely(nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
>> >> > - last_action(a, rem)))
>> >> > + nla_is_last(a, rem)))
>> >> > return output_userspace(dp, skb, key, a);
>> >> >
>> >> > skb = skb_clone(skb, GFP_ATOMIC);
>> >> > @@ -810,7 +805,7 @@ static int execute_recirc(struct datapath *dp,
>> >> > struct sk_buff *skb,
>> >> > }
>> >> > BUG_ON(!is_flow_key_valid(key));
>> >> >
>> >> > - if (!last_action(a, rem)) {
>> >> > + if (!nla_is_last(a, rem)) {
>> >> > /* Recirc action is the not the last action
>> >> > * of the action list, need to clone the skb.
>> >> > */
>> >> > @@ -897,7 +892,7 @@ static int do_execute_actions(struct datapath
>> >> > *dp, struct sk_buff *skb,
>> >> >
>> >> > case OVS_ACTION_ATTR_RECIRC:
>> >> > err = execute_recirc(dp, skb, key, a, rem);
>> >> > - if (last_action(a, rem)) {
>> >> > + if (nla_is_last(a, rem)) {
>> >> > /* If this is the last action, the
>> >> > skb has
>> >> > * been consumed or freed.
>> >> > * Return immediately.
>> >> > diff --git a/datapath/linux/compat/include/net/netlink.h
>> >> > b/datapath/linux/compat/include/net/netlink.h
>> >> > index a6dc584..de37058 100644
>> >> > --- a/datapath/linux/compat/include/net/netlink.h
>> >> > +++ b/datapath/linux/compat/include/net/netlink.h
>> >> > @@ -63,4 +63,8 @@ static inline struct nlattr *nla_find_nested(struct
>> >> > nlattr *nla, int attrtype)
>> >> > }
>> >> > #endif
>> >> >
>> >> > +static inline bool nla_is_last(const struct nlattr *a, int rem)
>> >> > +{
>> >> > + return a->nla_len == rem;
>> >> > +}
>> >> > #endif /* net/netlink.h */
>> >> > --
>> >> > 2.0.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] Add OpenFlow support for 802.1AD Ethertype
On Fri, Oct 17, 2014 at 01:19:33PM -0400, Dave Benson wrote: > On Fri, Oct 10, 2014 at 6:43 PM, Ben Pfaff wrote: > > With this change, I think that an OpenFlow flow table entry that > > matches on a VLAN will now match both 0x88a8 and 0x8100 tags for that > > VLAN. I doubt that that is desirable behavior, because my > > understanding is that 0x88a8 and 0x8100 tags are generally in > > different namespaces and are not interchangeable. > > Definately Agree. I saw that the OF spec has the same problem. They > define the eth_type as 'Ethernet type of the OpenFlow packet payload, after > VLAN tags.' So there is nothing currently to match on VLAN tpid. Does OVS > go it's own way in a case like this, or discuss this with the OF crowd? I think we should figure out what we want to do first, then propose it for OpenFlow afterward. I am involved enough in OpenFlow standardization that I have an idea what will go over well. My first thought is to define a new NXM_NX_VLAN_TPID that matches the TPID, and for backward compatibility to imply a match on 0x8100 if it is not given. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v5] datapath-windows: netdev-windows fixes for vswitchd bringup.
On Fri, Oct 17, 2014 at 11:12:32AM -0700, Ankur Sharma wrote: > Added the handlers for update_flags and set_etheraddr. > These handlers were needed for vswitchd bringup on windows > platform. > > Signed-off-by: Ankur Sharma > Acked-by: Nithin Raju Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath-windows: Packet subscribe handler
On Thu, Oct 16, 2014 at 11:45:42PM -0700, Eitan Eliahu wrote: > This change includes the following: > [1] Handler for subscribe/unsubscribe to a packet queue associated with a > socket pid. > [2] Allocation of per socket packet queue on a packet subscription. > [3] Removal of static allocated queues. > [4] Freeing the packet queue (on user mode process termination). > > Signed-off-by: Eitan Eliahu > Acked-by: Ankur Sharma Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] lib/dpif-netdev: Fix EMC lookup.
Patch 0de8783a9 (lib/dpif-netdev: Integrate megaflow classifier.)
broke exact match cache lookup, but it went undetected since there are
no separate stats for EMC.
This patch fixes the problem by changing the struct netdev_flow_key
'len' member to cover only the 'mf' member, not the whole
netdev_flow_key, and ignoring the 'len' field in
netdev_flow_key_equal. Comparison is still accurate, as the miniflow
'map' field encodes the length in the number of 1-bits, and the map is
included in the comparison.
Reported-by: Alex Wang
Signed-off-by: Jarno Rajahalme
---
lib/dpif-netdev.c | 26 ++
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index f6a0c48..8c8e6c5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -433,10 +433,13 @@ emc_cache_init(struct emc_cache *flow_cache)
{
int i;
+BUILD_ASSERT(offsetof(struct netdev_flow_key, mf) == 2 * sizeof(uint32_t));
+
for (i = 0; i < ARRAY_SIZE(flow_cache->entries); i++) {
flow_cache->entries[i].flow = NULL;
flow_cache->entries[i].key.hash = 0;
-flow_cache->entries[i].key.len = 0;
+flow_cache->entries[i].key.len
+= offsetof(struct miniflow, inline_values);
miniflow_initialize(&flow_cache->entries[i].key.mf,
flow_cache->entries[i].key.buf);
}
@@ -1269,11 +1272,11 @@ BUILD_ASSERT_DECL(offsetof(struct miniflow,
inline_values)
== sizeof(uint64_t));
/* Given the number of bits set in the miniflow map, returns the size of the
- * netdev_flow key */
+ * 'netdev_flow_key.mf' */
static inline uint32_t
netdev_flow_key_size(uint32_t flow_u32s)
{
-return offsetof(struct netdev_flow_key, mf.inline_values) +
+return offsetof(struct miniflow, inline_values) +
MINIFLOW_VALUES_SIZE(flow_u32s);
}
@@ -1281,8 +1284,8 @@ static inline bool
netdev_flow_key_equal(const struct netdev_flow_key *a,
const struct netdev_flow_key *b)
{
-/* 'b's size and hash may be not set yet. */
-return !memcmp(a, b, a->len);
+/* 'b->len' may be not set yet. */
+return a->hash == b->hash && !memcmp(&a->mf, &b->mf, a->len);
}
/* Used to compare 'netdev_flow_key' in the exact match cache to a miniflow.
@@ -1292,15 +1295,15 @@ static inline bool
netdev_flow_key_equal_mf(const struct netdev_flow_key *key,
const struct miniflow *mf)
{
-return !memcmp(&key->mf, mf,
- key->len - offsetof(struct netdev_flow_key, mf));
+return !memcmp(&key->mf, mf, key->len);
}
static inline void
netdev_flow_key_clone(struct netdev_flow_key *dst,
const struct netdev_flow_key *src)
{
-memcpy(dst, src, src->len);
+memcpy(dst, src,
+ offsetof(struct netdev_flow_key, mf) + src->len);
}
/* Slow. */
@@ -1685,7 +1688,7 @@ dp_netdev_flow_add(struct dp_netdev *dp, struct match
*match,
ovs_assert(!(mask.mf.map & (MINIFLOW_MAP(metadata) | MINIFLOW_MAP(regs;
/* Do not allocate extra space. */
-flow = xmalloc(sizeof *flow - sizeof flow->cr.flow + mask.len);
+flow = xmalloc(sizeof *flow - sizeof flow->cr.flow.mf + mask.len);
flow->dead = false;
*CONST_CAST(struct flow *, &flow->flow) = match->flow;
ovs_refcount_init(&flow->ref_cnt);
@@ -2840,8 +2843,6 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
}
ovs_mutex_unlock(&dp->flow_mutex);
-/* EMC uses different hash. */
-keys[i].hash = dpif_packet_get_dp_hash(packets[i]);
emc_insert(flow_cache, &keys[i], netdev_flow);
}
}
@@ -3277,7 +3278,8 @@ dpcls_create_subtable(struct dpcls *cls, const struct
netdev_flow_key *mask)
struct dpcls_subtable *subtable;
/* Need to add one. */
-subtable = xmalloc(sizeof *subtable - sizeof subtable->mask + mask->len);
+subtable = xmalloc(sizeof *subtable
+ - sizeof subtable->mask.mf + mask->len);
cmap_init(&subtable->rules);
netdev_flow_key_clone(&subtable->mask, mask);
cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash);
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] stream-tcp: Call setsockopt TCP_NODELAY after TCP is connected for Windows.
On Fri, Oct 17, 2014 at 01:23:04PM -0700, Gurucharan Shetty wrote: > On Tue, Oct 7, 2014 at 2:25 PM, Ben Pfaff wrote: > > On Thu, Oct 02, 2014 at 08:16:56AM -0700, Gurucharan Shetty wrote: > >> On Windows platform, TCP_NODELAY can only be set when TCP is established. > >> The current code does not create any problems while running unit tests > >> (because connections get established immediately) but is observed while > >> connecting to a different machine. > >> > >> commit 8b76839(Move setsockopt TCP_NODELAY to when TCP is connected.) > >> made changes to call setsockopt with TCP_NODELAY after TCP is connected > >> only in lib/stream-ssl.c. We need the same change for stream-tcp too. > >> > >> Signed-off-by: Gurucharan Shetty > > > > I guess we will need a similar change for stream-ssl.c. > As noted in the commit message, stream-ssl already has the change. > stream-tcp was missing it. Oh, I see now. I saw that new_ssl_stream() called setsockopt_tcp_nodelay(), with a comment that it didn't work on Windows, and didn't fully catch on. > > I'm not a fan of unnecessary platform-related differences. That makes > > me think that it might be better to set TCP_NODELAY only after > > connecting on every platform. After all, there is no benefit, except > > simplicity, to setting it before one can actually send any data, and > > we cannot get the simplicity benefit anyway. > > > > The one complication is that on Windows, stream-fd is only used for > > TCP sockets, whereas on Unix it is used for Unix domain sockets and > > TCP sockets, and one cannot set TCP_NODELAY on a non-TCP socket. I > > guess that means that we'd need a new parameter to new_fd_stream() to > > indicate whether it is a TCP socket. That's probably OK though. > > > > Looking at a diff, the differences between the two versions of > > stream-fd are not large, and some of the differences are unnecessary. > > Maybe this change is an argument toward re-unification. > I agree. As a first step, I will send in a patch that merges > stream-fd-windows and stream-fd-unix. Once that looks fine, I will > resping this patch. Great. I'll read the other patch now. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] stream-fd: Merge stream-fd-windows and stream-fd-unix.
On Fri, Oct 17, 2014 at 12:25:41PM -0700, Gurucharan Shetty wrote: > There was not much difference between the two files after moving > all of the Windows socket HANDLE polling functionality to poll-loop.c. > So merge them together. > > Signed-off-by: Gurucharan Shetty Besides reading the code, I ran the unit tests on GNU/Linux. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] lib/dpif-netdev: Fix EMC lookup.
Thanks for fixing this Jarno!
I've tested the patch and it fixes the problem.
Would you mind adding a comment on struct netdev_flow_key that 'len'
accounts for the length of the miniflow only (including the map)?
Other comments inline, otherwise:
Acked-by: Daniele Di Proietto
On 10/17/14, 3:05 PM, "Jarno Rajahalme" wrote:
>Patch 0de8783a9 (lib/dpif-netdev: Integrate megaflow classifier.)
>broke exact match cache lookup, but it went undetected since there are
>no separate stats for EMC.
>
>This patch fixes the problem by changing the struct netdev_flow_key
>'len' member to cover only the 'mf' member, not the whole
>netdev_flow_key, and ignoring the 'len' field in
>netdev_flow_key_equal. Comparison is still accurate, as the miniflow
>'map' field encodes the length in the number of 1-bits, and the map is
>included in the comparison.
>
>Reported-by: Alex Wang
>Signed-off-by: Jarno Rajahalme
>---
> lib/dpif-netdev.c | 26 ++
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>index f6a0c48..8c8e6c5 100644
>--- a/lib/dpif-netdev.c
>+++ b/lib/dpif-netdev.c
>@@ -433,10 +433,13 @@ emc_cache_init(struct emc_cache *flow_cache)
> {
> int i;
>
>+BUILD_ASSERT(offsetof(struct netdev_flow_key, mf) == 2 *
>sizeof(uint32_t));
>+
I believe now we should assert that offsetof(struct miniflow,
inline_values) == sizeof(uint64_t).
> for (i = 0; i < ARRAY_SIZE(flow_cache->entries); i++) {
> flow_cache->entries[i].flow = NULL;
> flow_cache->entries[i].key.hash = 0;
>-flow_cache->entries[i].key.len = 0;
>+flow_cache->entries[i].key.len
>+= offsetof(struct miniflow, inline_values);
> miniflow_initialize(&flow_cache->entries[i].key.mf,
> flow_cache->entries[i].key.buf);
> }
>@@ -1269,11 +1272,11 @@ BUILD_ASSERT_DECL(offsetof(struct miniflow,
>inline_values)
> == sizeof(uint64_t));
>
> /* Given the number of bits set in the miniflow map, returns the size of
>the
>- * netdev_flow key */
>+ * 'netdev_flow_key.mf' */
> static inline uint32_t
> netdev_flow_key_size(uint32_t flow_u32s)
> {
>-return offsetof(struct netdev_flow_key, mf.inline_values) +
>+return offsetof(struct miniflow, inline_values) +
> MINIFLOW_VALUES_SIZE(flow_u32s);
> }
>
>@@ -1281,8 +1284,8 @@ static inline bool
> netdev_flow_key_equal(const struct netdev_flow_key *a,
> const struct netdev_flow_key *b)
> {
>-/* 'b's size and hash may be not set yet. */
>-return !memcmp(a, b, a->len);
>+/* 'b->len' may be not set yet. */
>+return a->hash == b->hash && !memcmp(&a->mf, &b->mf, a->len);
> }
>
> /* Used to compare 'netdev_flow_key' in the exact match cache to a
>miniflow.
>@@ -1292,15 +1295,15 @@ static inline bool
> netdev_flow_key_equal_mf(const struct netdev_flow_key *key,
> const struct miniflow *mf)
> {
>-return !memcmp(&key->mf, mf,
>- key->len - offsetof(struct netdev_flow_key, mf));
>+return !memcmp(&key->mf, mf, key->len);
> }
>
> static inline void
> netdev_flow_key_clone(struct netdev_flow_key *dst,
> const struct netdev_flow_key *src)
> {
>-memcpy(dst, src, src->len);
>+memcpy(dst, src,
>+ offsetof(struct netdev_flow_key, mf) + src->len);
> }
>
> /* Slow. */
>@@ -1685,7 +1688,7 @@ dp_netdev_flow_add(struct dp_netdev *dp, struct
>match *match,
> ovs_assert(!(mask.mf.map & (MINIFLOW_MAP(metadata) |
>MINIFLOW_MAP(regs;
>
> /* Do not allocate extra space. */
>-flow = xmalloc(sizeof *flow - sizeof flow->cr.flow + mask.len);
>+flow = xmalloc(sizeof *flow - sizeof flow->cr.flow.mf + mask.len);
> flow->dead = false;
> *CONST_CAST(struct flow *, &flow->flow) = match->flow;
> ovs_refcount_init(&flow->ref_cnt);
>@@ -2840,8 +2843,6 @@ fast_path_processing(struct dp_netdev_pmd_thread
>*pmd,
> }
> ovs_mutex_unlock(&dp->flow_mutex);
>
>-/* EMC uses different hash. */
>-keys[i].hash = dpif_packet_get_dp_hash(packets[i]);
There's another emc_insert call right below this. Would you mind removing
the same line before that one too?
> emc_insert(flow_cache, &keys[i], netdev_flow);
> }
> }
>@@ -3277,7 +3278,8 @@ dpcls_create_subtable(struct dpcls *cls, const
>struct netdev_flow_key *mask)
> struct dpcls_subtable *subtable;
>
> /* Need to add one. */
>-subtable = xmalloc(sizeof *subtable - sizeof subtable->mask +
>mask->len);
>+subtable = xmalloc(sizeof *subtable
>+ - sizeof subtable->mask.mf + mask->len);
> cmap_init(&subtable->rules);
> netdev_flow_key_clone(&subtable->mask, mask);
> cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash);
>--
>1.7.10.4
>
>___
>dev mailing list
>dev@openvswitch.o
[ovs-dev] [PATCH] lib/dpif: Fix crash in format_odp_actions, actions = 0x0
dpif_flow_get initializes the flow_get part of the union, down the stack log_flow_message checks for actions || actions_len that could contain garbage leading to the crash. saw the crash once when running stress tests. can be easily recreated by running ovs-dpctl del-flows in a loop when traffic is going on Signed-off-by: Madhu Challa --- lib/dpif.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/dpif.c b/lib/dpif.c index d088f68..72ae2d4 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -840,6 +840,8 @@ dpif_flow_get(struct dpif *dpif, struct dpif_op *opp; struct dpif_op op; +memset(&op, 0, sizeof op); + op.type = DPIF_OP_FLOW_GET; op.u.flow_get.key = key; op.u.flow_get.key_len = key_len; -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] lib/dpif: Fix crash in format_odp_actions, actions = 0x0
On Fri, Oct 17, 2014 at 04:07:25PM -0700, Madhu Challa wrote:
> dpif_flow_get initializes the flow_get part of the union, down the stack
> log_flow_message checks for actions || actions_len that could contain
> garbage leading to the crash.
>
> saw the crash once when running stress tests. can be easily recreated
> by running ovs-dpctl del-flows in a loop when traffic is going on
>
> Signed-off-by: Madhu Challa
The actions aren't in the dpif_op so I don't see how this would help.
Can you explain?
The actions are, instead, in the caller-provided dpif_flow. I guess
that the error is here in dpif_operate() where the code clears the
flow only after trying to log uninitialized garbage from it:
log_flow_get_message(dpif, get, error);
if (error) {
memset(get->flow, 0, sizeof *get->flow);
}
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] lib/dpif: Fix crash in format_odp_actions, actions = 0x0
You are correct Ben. I confused with the put that has actions in
dpif_flow_put. I guess I was not able to repro the issue then. Let me repro
it and I will resend the fix.
Thanks.
On Fri, Oct 17, 2014 at 4:17 PM, Ben Pfaff wrote:
> On Fri, Oct 17, 2014 at 04:07:25PM -0700, Madhu Challa wrote:
> > dpif_flow_get initializes the flow_get part of the union, down the stack
> > log_flow_message checks for actions || actions_len that could contain
> > garbage leading to the crash.
> >
> > saw the crash once when running stress tests. can be easily recreated
> > by running ovs-dpctl del-flows in a loop when traffic is going on
> >
> > Signed-off-by: Madhu Challa
>
> The actions aren't in the dpif_op so I don't see how this would help.
> Can you explain?
>
> The actions are, instead, in the caller-provided dpif_flow. I guess
> that the error is here in dpif_operate() where the code clears the
> flow only after trying to log uninitialized garbage from it:
> log_flow_get_message(dpif, get, error);
>
> if (error) {
> memset(get->flow, 0, sizeof *get->flow);
> }
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] lib/dpif: Fix crash in format_odp_actions, actions = 0x0
Forgot to attach the backtrace ::
(gdb) bt
#0 nl_attr_is_valid (maxlen=0xcfea1104, nla=0x0) at lib/netlink.h:139
#1 format_odp_actions (ds=0x7f77c7ffc800, actions=0x0,
actions_len=0x7f77cfea1104) at lib/odp-util.c:608
#2 0x00456749 in log_flow_message (error=0x2,
operation=0x506548 "flow_get", key=0x7f779405ac30, key_len=0x60,
mask=0x7f77ac000ad0, mask_len=0x78f1e0, stats=0x7f77c7ffeb50,
actions=0x0,
actions_len=0x7f77cfea1104, dpif=) at lib/dpif.c:1498
#3 0x00456de8 in log_flow_get_message (error=0x2,
get=0x7f77c7ffc9a8,
dpif=0x19178f0) at lib/dpif.c:1588
#4 dpif_operate (dpif=0x19178f0, ops=0x7f77c7ffc9f8, n_ops=0x1)
at lib/dpif.c:1158
#5 0x004572ce in dpif_flow_get (dpif=,
key=, key_len=, buf=,
flow=) at lib/dpif.c:852
#6 0x00431514 in handle_missed_revalidation (ukey=0x7f779405aba0,
revalidator=) at ofproto/ofproto-dpif-upcall.c:1609
#7 revalidator_sweep__ (revalidator=0x19592f0, purge=0x0)
at ofproto/ofproto-dpif-upcall.c:1636
#8 0x00431da7 in revalidator_sweep (revalidator=0x19592f0)
at ofproto/ofproto-dpif-upcall.c:1657
#9 udpif_revalidator (arg=0x19592f0) at ofproto/ofproto-dpif-upcall.c:705
#10 0x0049e652 in ovsthread_wrapper (aux_=)
at lib/ovs-thread.c:338
#11 0x7f77cfe9de9a in start_thread (arg=0x7f77c7fff700)
at pthread_create.c:308
#12 0x7f77cf6c63fd in clone ()
at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#13 0x in ?? ()
On Fri, Oct 17, 2014 at 4:25 PM, Madhu Challa
wrote:
> You are correct Ben. I confused with the put that has actions in
> dpif_flow_put. I guess I was not able to repro the issue then. Let me repro
> it and I will resend the fix.
>
> Thanks.
>
> On Fri, Oct 17, 2014 at 4:17 PM, Ben Pfaff wrote:
>
>> On Fri, Oct 17, 2014 at 04:07:25PM -0700, Madhu Challa wrote:
>> > dpif_flow_get initializes the flow_get part of the union, down the stack
>> > log_flow_message checks for actions || actions_len that could contain
>> > garbage leading to the crash.
>> >
>> > saw the crash once when running stress tests. can be easily recreated
>> > by running ovs-dpctl del-flows in a loop when traffic is going on
>> >
>> > Signed-off-by: Madhu Challa
>>
>> The actions aren't in the dpif_op so I don't see how this would help.
>> Can you explain?
>>
>> The actions are, instead, in the caller-provided dpif_flow. I guess
>> that the error is here in dpif_operate() where the code clears the
>> flow only after trying to log uninitialized garbage from it:
>> log_flow_get_message(dpif, get, error);
>>
>> if (error) {
>> memset(get->flow, 0, sizeof *get->flow);
>> }
>>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/1] netdev-dpdk: Move to DPDK 1.7.1
Thanks for the patch.
Acked-by: Daniele Di Proietto
Pravin, would you mind pushing this? I suggest one minor change, inline.
On 10/13/14, 7:17 AM, "maryam.tahhan" wrote:
>This patch updates the documentation to reflect that DPDK 1.7.1 is
>supported.
>Travis scripts have also been updated to reflect this. DPDK phy and ring
>ports
>were validated against DPDK 1.7.1.
>(note: ring ports were validated with the upcoming multi-queue patch fix).
>
>Reviewed-by: Mark D. Gray
>Signed-off-by: Maryam Tahhan
>---
> .travis.yml | 2 +-
> .travis/build.sh | 6 +++---
> INSTALL.DPDK | 4 ++--
> acinclude.m4 | 2 +-
> 4 files changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/.travis.yml b/.travis.yml
>index 4dfe15e..cd6e623 100644
>--- a/.travis.yml
>+++ b/.travis.yml
>@@ -8,7 +8,7 @@ before_install: ./.travis/prepare.sh
> env:
> - OPTS="--disable-ssl"
> - TESTSUITE=1 KERNEL=1 OPTS="--with-linux=./linux-3.16.2"
>- - KERNEL=1 DPDK=1 OPTS="--with-dpdk=./dpdk-1.7.0/build"
>+ - KERNEL=1 DPDK=1 OPTS="--with-dpdk=./dpdk-1.7.1/build"
>
> script: ./.travis/build.sh $OPTS
>
>diff --git a/.travis/build.sh b/.travis/build.sh
>index 3872893..5dba4fe 100755
>--- a/.travis/build.sh
>+++ b/.travis/build.sh
>@@ -19,9 +19,9 @@ function install_kernel()
>
> function install_dpdk()
> {
>-wget
>https://urldefense.proofpoint.com/v1/url?u=http://www.dpdk.org/browse/dpdk
>/snapshot/dpdk-1.7.0.tar.gz&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjt
>FIdhBDBaw5z%2BU6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=GQUPdW%2FL5fNkvOTC0RUDpJXz
>qsWaHRpGLnSKLH4paEI%3D%0A&s=c8dc9c6bb356802da33c5b91e88404ee67d81653868939
>c51b7e2cc4beb688d5
>-tar xzvf dpdk-1.7.0.tar.gz > /dev/null
>-cd dpdk-1.7.0
>+wget
>https://urldefense.proofpoint.com/v1/url?u=http://www.dpdk.org/browse/dpdk
>/snapshot/dpdk-1.7.1.tar.gz&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjt
>FIdhBDBaw5z%2BU6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=GQUPdW%2FL5fNkvOTC0RUDpJXz
>qsWaHRpGLnSKLH4paEI%3D%0A&s=840ce9c562b5a4da31596fa459d50ec10b38066f49231c
>85f71abe9fb7b6ac81
>+tar xzvf dpdk-1.7.1.tar.gz > /dev/null
>+cd dpdk-1.7.1
> find ./ -type f | xargs sed -i
>'s/max-inline-insns-single=100/max-inline-insns-single=400/'
> sed -ri 's,(CONFIG_RTE_BUILD_COMBINE_LIBS=).*,\1y,'
>config/common_linuxapp
> make config CC=gcc T=x86_64-native-linuxapp-gcc
>diff --git a/INSTALL.DPDK b/INSTALL.DPDK
>index d9a77c9..7484b4b 100644
>--- a/INSTALL.DPDK
>+++ b/INSTALL.DPDK
>@@ -14,10 +14,10 @@ and "make".
> Building and Installing:
>
>
>-Required DPDK 1.7.
>+Required DPDK 1.7.1
We do not require DPDK 1.7.1. OVS works even with 1.7.0. The reason I'm
saying this is that some distro might have packaged DPDK 1.7.0.
>
> DPDK:
>-Set dir i.g.: export DPDK_DIR=/usr/src/dpdk-1.7.0
>+Set dir i.g.: export DPDK_DIR=/usr/src/dpdk-1.7.1
> cd $DPDK_DIR
> update config/common_linuxapp so that dpdk generate single lib file.
> (modification also required for IVSHMEM build)
>diff --git a/acinclude.m4 b/acinclude.m4
>index 9a7f809..f9ee2ec 100644
>--- a/acinclude.m4
>+++ b/acinclude.m4
>@@ -206,7 +206,7 @@ AC_DEFUN([OVS_CHECK_DPDK], [
> OVS_LDFLAGS="$OVS_LDFLAGS -L$DPDK_LIB_DIR"
> OVS_CFLAGS="$OVS_CFLAGS -I$DPDK_INCLUDE"
>
>-# DPDK 1.7.0 pmd drivers are not linked unless --whole-archive is
>used.
>+# DPDK 1.7.1 pmd drivers are not linked unless --whole-archive is
>used.
> #
> # This happens because the rest of the DPDK code doesn't use any
>symbol in
> # the pmd driver objects, and the drivers register themselves using
>an
>--
>1.9.0
>
>___
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/
>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5z%2BU
>6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=GQUPdW%2FL5fNkvOTC0RUDpJXzqsWaHRpGLnSKLH4
>paEI%3D%0A&s=e5effbefba9164425ea4eede02252eaea97dc640a221b2f062e73c57da2c0
>2e7
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] INSTALL.DPDK: improve documentation
Signed-off-by: Daniele Di Proietto --- INSTALL.DPDK | 40 +--- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/INSTALL.DPDK b/INSTALL.DPDK index 7484b4b..0d19012 100644 --- a/INSTALL.DPDK +++ b/INSTALL.DPDK @@ -11,6 +11,8 @@ It has not been thoroughly tested. This version of Open vSwitch should be built manually with "configure" and "make". +OVS needs a system with 1GB hugepages support. + Building and Installing: @@ -44,6 +46,12 @@ cd $(OVS_DIR)/openvswitch ./configure --with-dpdk=$DPDK_BUILD make +To have better performance one can enable aggressive compiler optimizations and +use the special instructions(popcnt, crc32) that may not be available on all +machines. Instead of typing 'make', type: + +make CFLAGS='-O3 -march=native' + Refer to INSTALL.userspace for general requirements of building userspace OVS. @@ -60,24 +68,6 @@ First setup DPDK devices: e.g. insmod $DPDK_BUILD/kmod/igb_uio.ko - Bind network device to igb_uio. e.g. $DPDK_DIR/tools/dpdk_nic_bind.py --bind=igb_uio eth1 -Alternate binding method: - Find target Ethernet devices - lspci -nn|grep Ethernet - Bring Down (e.g. eth2, eth3) - ifconfig eth2 down - ifconfig eth3 down - Look at current devices (e.g ixgbe devices) - ls /sys/bus/pci/drivers/ixgbe/ - :02:00.0 :02:00.1 bind module new_id remove_id uevent unbind - Unbind target pci devices from current driver (e.g. 02:00.0 ...) - echo :02:00.0 > /sys/bus/pci/drivers/ixgbe/unbind - echo :02:00.1 > /sys/bus/pci/drivers/ixgbe/unbind - Bind to target driver (e.g. igb_uio) - echo :02:00.0 > /sys/bus/pci/drivers/igb_uio/bind - echo :02:00.1 > /sys/bus/pci/drivers/igb_uio/bind - Check binding for listed devices - ls /sys/bus/pci/drivers/igb_uio - :02:00.0 :02:00.1 bind module new_id remove_id uevent unbind Prepare system: - mount hugetlbfs @@ -124,11 +114,11 @@ node 0 memory: To use ovs-vswitchd with DPDK, create a bridge with datapath_type "netdev" in the configuration database. For example: -ovs-vsctl add-br br0 -ovs-vsctl set bridge br0 datapath_type=netdev +ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev Now you can add dpdk devices. OVS expect DPDK device name start with dpdk -and end with portid. vswitchd should print number of dpdk devices found. +and end with portid. vswitchd should print (in the log file) the number of dpdk +devices found. ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk ovs-vsctl add-port br0 dpdk1 -- set Interface dpdk1 type=dpdk @@ -138,8 +128,6 @@ polls dpdk device in continuous loop. Therefore CPU utilization for that thread is always 100%. Test flow script across NICs (assuming ovs in /usr/src/ovs): - Assume 1.1.1.1 on NIC port 1 (dpdk0) - Assume 1.1.1.2 on NIC port 2 (dpdk1) Execute script: # Script: @@ -152,10 +140,8 @@ cd /usr/src/ovs/utilities/ ./ovs-ofctl del-flows br0 # Add flows between port 1 (dpdk0) to port 2 (dpdk1) -./ovs-ofctl add-flow br0 in_port=1,dl_type=0x800,nw_src=1.1.1.1,\ -nw_dst=1.1.1.2,idle_timeout=0,action=output:2 -./ovs-ofctl add-flow br0 in_port=2,dl_type=0x800,nw_src=1.1.1.2,\ -nw_dst=1.1.1.1,idle_timeout=0,action=output:1 +./ovs-ofctl add-flow br0 in_port=1,action=output:2 +./ovs-ofctl add-flow br0 in_port=2,action=output:1 ## -- 2.1.0.rc1 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] lib/dpif-netdev: Fix EMC lookup.
Pushed, thanks for the review!
Jarno
On Oct 17, 2014, at 4:05 PM, Daniele Di Proietto wrote:
> Thanks for fixing this Jarno!
>
> I've tested the patch and it fixes the problem.
> Would you mind adding a comment on struct netdev_flow_key that 'len'
> accounts for the length of the miniflow only (including the map)?
> Other comments inline, otherwise:
>
> Acked-by: Daniele Di Proietto
>
> On 10/17/14, 3:05 PM, "Jarno Rajahalme" wrote:
>
>> Patch 0de8783a9 (lib/dpif-netdev: Integrate megaflow classifier.)
>> broke exact match cache lookup, but it went undetected since there are
>> no separate stats for EMC.
>>
>> This patch fixes the problem by changing the struct netdev_flow_key
>> 'len' member to cover only the 'mf' member, not the whole
>> netdev_flow_key, and ignoring the 'len' field in
>> netdev_flow_key_equal. Comparison is still accurate, as the miniflow
>> 'map' field encodes the length in the number of 1-bits, and the map is
>> included in the comparison.
>>
>> Reported-by: Alex Wang
>> Signed-off-by: Jarno Rajahalme
>> ---
>> lib/dpif-netdev.c | 26 ++
>> 1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index f6a0c48..8c8e6c5 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -433,10 +433,13 @@ emc_cache_init(struct emc_cache *flow_cache)
>> {
>>int i;
>>
>> +BUILD_ASSERT(offsetof(struct netdev_flow_key, mf) == 2 *
>> sizeof(uint32_t));
>> +
>
> I believe now we should assert that offsetof(struct miniflow,
> inline_values) == sizeof(uint64_t).
>
>>for (i = 0; i < ARRAY_SIZE(flow_cache->entries); i++) {
>>flow_cache->entries[i].flow = NULL;
>>flow_cache->entries[i].key.hash = 0;
>> -flow_cache->entries[i].key.len = 0;
>> +flow_cache->entries[i].key.len
>> += offsetof(struct miniflow, inline_values);
>>miniflow_initialize(&flow_cache->entries[i].key.mf,
>>flow_cache->entries[i].key.buf);
>>}
>> @@ -1269,11 +1272,11 @@ BUILD_ASSERT_DECL(offsetof(struct miniflow,
>> inline_values)
>> == sizeof(uint64_t));
>>
>> /* Given the number of bits set in the miniflow map, returns the size of
>> the
>> - * netdev_flow key */
>> + * 'netdev_flow_key.mf' */
>> static inline uint32_t
>> netdev_flow_key_size(uint32_t flow_u32s)
>> {
>> -return offsetof(struct netdev_flow_key, mf.inline_values) +
>> +return offsetof(struct miniflow, inline_values) +
>>MINIFLOW_VALUES_SIZE(flow_u32s);
>> }
>>
>> @@ -1281,8 +1284,8 @@ static inline bool
>> netdev_flow_key_equal(const struct netdev_flow_key *a,
>> const struct netdev_flow_key *b)
>> {
>> -/* 'b's size and hash may be not set yet. */
>> -return !memcmp(a, b, a->len);
>> +/* 'b->len' may be not set yet. */
>> +return a->hash == b->hash && !memcmp(&a->mf, &b->mf, a->len);
>> }
>>
>> /* Used to compare 'netdev_flow_key' in the exact match cache to a
>> miniflow.
>> @@ -1292,15 +1295,15 @@ static inline bool
>> netdev_flow_key_equal_mf(const struct netdev_flow_key *key,
>> const struct miniflow *mf)
>> {
>> -return !memcmp(&key->mf, mf,
>> - key->len - offsetof(struct netdev_flow_key, mf));
>> +return !memcmp(&key->mf, mf, key->len);
>> }
>>
>> static inline void
>> netdev_flow_key_clone(struct netdev_flow_key *dst,
>> const struct netdev_flow_key *src)
>> {
>> -memcpy(dst, src, src->len);
>> +memcpy(dst, src,
>> + offsetof(struct netdev_flow_key, mf) + src->len);
>> }
>>
>> /* Slow. */
>> @@ -1685,7 +1688,7 @@ dp_netdev_flow_add(struct dp_netdev *dp, struct
>> match *match,
>>ovs_assert(!(mask.mf.map & (MINIFLOW_MAP(metadata) |
>> MINIFLOW_MAP(regs;
>>
>>/* Do not allocate extra space. */
>> -flow = xmalloc(sizeof *flow - sizeof flow->cr.flow + mask.len);
>> +flow = xmalloc(sizeof *flow - sizeof flow->cr.flow.mf + mask.len);
>>flow->dead = false;
>>*CONST_CAST(struct flow *, &flow->flow) = match->flow;
>>ovs_refcount_init(&flow->ref_cnt);
>> @@ -2840,8 +2843,6 @@ fast_path_processing(struct dp_netdev_pmd_thread
>> *pmd,
>>}
>>ovs_mutex_unlock(&dp->flow_mutex);
>>
>> -/* EMC uses different hash. */
>> -keys[i].hash = dpif_packet_get_dp_hash(packets[i]);
>
> There's another emc_insert call right below this. Would you mind removing
> the same line before that one too?
>
>>emc_insert(flow_cache, &keys[i], netdev_flow);
>>}
>>}
>> @@ -3277,7 +3278,8 @@ dpcls_create_subtable(struct dpcls *cls, const
>> struct netdev_flow_key *mask)
>>struct dpcls_subtable *subtable;
>>
>>/* Need to add one. */
>> -subtable = xmalloc(sizeof *subtable - sizeof subtable->mask +
>> mask->len);
>> +subtable = xmalloc(sizeof *subtable
>> + - sizeof subtabl
Re: [ovs-dev] [PATCH] lib/dpif: Fix crash in format_odp_actions, actions = 0x0
I am able to recreate it. I suspect its some bug at the dpif_netlink layer.
I am trying to narrow it down further. Anything in particular I could look
for ?
Thanks.
(gdb) print *get
$3 = {
key = 0x7fedec092190,
key_len = 0x60,
buffer = 0x7fee162a4b70,
flow = 0x7fee162a4b20
}
(gdb) print *get->flow
$4 = {
key = 0x7fedec092190,
key_len = 0x60,
mask = 0x7fedf4000ac0,
mask_len = 0x458fc3,
actions = 0x7fee162a5900,
actions_len = 0x49e690,
stats = {
n_packets = 0x7fedf4000ac0,
n_bytes = 0x6d506b38dcb0ac00,
used = 0x7fedf4000ac0,
tcp_flags = 0xe958
}
}
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fee162a5700 (LWP 10592)]
format_odp_actions (ds=0x7fee162a2800, actions=0x7fee162a5900,
actions_len=4843152) at lib/odp-util.c:622
622ds_put_format(ds, "%02x", ((const uint8_t *) a)[i]);
(gdb) bt
#0 format_odp_actions (ds=0x7fee162a2800, actions=0x7fee162a5900,
actions_len=4843152) at lib/odp-util.c:622
#1 0x00456749 in log_flow_message (error=2, operation=0x506548
"flow_get", key=0x7fedec092190, key_len=96,
mask=0x7fedf4000ac0, mask_len=4558787, stats=0x7fee162a4b50,
actions=0x7fee162a5900, actions_len=4843152, dpif=)
at lib/dpif.c:1498
#2 0x00456de8 in log_flow_get_message (error=2,
get=0x7fee162a29a8, dpif=0x24db8f0) at lib/dpif.c:1588
#3 dpif_operate (dpif=0x24db8f0, ops=0x7fee162a29f8, n_ops=1) at
lib/dpif.c:1158
#4 0x004572ce in dpif_flow_get (dpif=,
key=, key_len=, buf=,
flow=) at lib/dpif.c:852
#5 0x00431514 in handle_missed_revalidation (ukey=0x7fedec092100,
revalidator=)
at ofproto/ofproto-dpif-upcall.c:1609
#6 revalidator_sweep__ (revalidator=0x251abb0, purge=false) at
ofproto/ofproto-dpif-upcall.c:1636
#7 0x00431da7 in revalidator_sweep (revalidator=0x251abb0) at
ofproto/ofproto-dpif-upcall.c:1657
#8 udpif_revalidator (arg=0x251abb0) at ofproto/ofproto-dpif-upcall.c:705
#9 0x0049e652 in ovsthread_wrapper (aux_=) at
lib/ovs-thread.c:338
#10 0x7fee19791e9a in start_thread (arg=0x7fee162a5700) at
pthread_create.c:308
#11 0x7fee18fba3fd in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#12 0x in ?? ()
On Fri, Oct 17, 2014 at 4:17 PM, Ben Pfaff wrote:
> On Fri, Oct 17, 2014 at 04:07:25PM -0700, Madhu Challa wrote:
> > dpif_flow_get initializes the flow_get part of the union, down the stack
> > log_flow_message checks for actions || actions_len that could contain
> > garbage leading to the crash.
> >
> > saw the crash once when running stress tests. can be easily recreated
> > by running ovs-dpctl del-flows in a loop when traffic is going on
> >
> > Signed-off-by: Madhu Challa
>
> The actions aren't in the dpif_op so I don't see how this would help.
> Can you explain?
>
> The actions are, instead, in the caller-provided dpif_flow. I guess
> that the error is here in dpif_operate() where the code clears the
> flow only after trying to log uninitialized garbage from it:
> log_flow_get_message(dpif, get, error);
>
> if (error) {
> memset(get->flow, 0, sizeof *get->flow);
> }
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] lib/dpif: Fix crash in format_odp_actions, actions = 0x0
I think I know whats happening. In case there is an error getting the
datapath flow we should not try logging it since the flow is not valid.
dpif_netlink_operate__
{
...
case DPIF_OP_FLOW_GET:
op->error = dpif_netlink_flow_from_ofpbuf(&reply,
txn->reply);
if (!op->error) {
dpif_netlink_flow_to_dpif_flow(get->flow, &reply);
}
I also checked the kernel datapath does not populate the flow in this case.
Pl let me know if it makes sense and I can submit a patch to fix the issue.
btw the repro is inconsistent, I have seen it a few times, I have not seen
it since I sent my last message.
Thanks.
Thanks.
On Fri, Oct 17, 2014 at 5:09 PM, Madhu Challa
wrote:
> I am able to recreate it. I suspect its some bug at the dpif_netlink
> layer. I am trying to narrow it down further. Anything in particular I
> could look for ?
>
> Thanks.
>
> (gdb) print *get
> $3 = {
> key = 0x7fedec092190,
> key_len = 0x60,
> buffer = 0x7fee162a4b70,
> flow = 0x7fee162a4b20
> }
> (gdb) print *get->flow
> $4 = {
> key = 0x7fedec092190,
> key_len = 0x60,
> mask = 0x7fedf4000ac0,
> mask_len = 0x458fc3,
> actions = 0x7fee162a5900,
> actions_len = 0x49e690,
> stats = {
> n_packets = 0x7fedf4000ac0,
> n_bytes = 0x6d506b38dcb0ac00,
> used = 0x7fedf4000ac0,
> tcp_flags = 0xe958
> }
> }
>
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fee162a5700 (LWP 10592)]
> format_odp_actions (ds=0x7fee162a2800, actions=0x7fee162a5900,
> actions_len=4843152) at lib/odp-util.c:622
> 622ds_put_format(ds, "%02x", ((const uint8_t *) a)[i]);
> (gdb) bt
> #0 format_odp_actions (ds=0x7fee162a2800, actions=0x7fee162a5900,
> actions_len=4843152) at lib/odp-util.c:622
> #1 0x00456749 in log_flow_message (error=2, operation=0x506548
> "flow_get", key=0x7fedec092190, key_len=96,
> mask=0x7fedf4000ac0, mask_len=4558787, stats=0x7fee162a4b50,
> actions=0x7fee162a5900, actions_len=4843152, dpif=)
> at lib/dpif.c:1498
> #2 0x00456de8 in log_flow_get_message (error=2,
> get=0x7fee162a29a8, dpif=0x24db8f0) at lib/dpif.c:1588
> #3 dpif_operate (dpif=0x24db8f0, ops=0x7fee162a29f8, n_ops=1) at
> lib/dpif.c:1158
> #4 0x004572ce in dpif_flow_get (dpif=,
> key=, key_len=, buf=,
> flow=) at lib/dpif.c:852
> #5 0x00431514 in handle_missed_revalidation (ukey=0x7fedec092100,
> revalidator=)
> at ofproto/ofproto-dpif-upcall.c:1609
> #6 revalidator_sweep__ (revalidator=0x251abb0, purge=false) at
> ofproto/ofproto-dpif-upcall.c:1636
> #7 0x00431da7 in revalidator_sweep (revalidator=0x251abb0) at
> ofproto/ofproto-dpif-upcall.c:1657
> #8 udpif_revalidator (arg=0x251abb0) at ofproto/ofproto-dpif-upcall.c:705
> #9 0x0049e652 in ovsthread_wrapper (aux_=) at
> lib/ovs-thread.c:338
> #10 0x7fee19791e9a in start_thread (arg=0x7fee162a5700) at
> pthread_create.c:308
> #11 0x7fee18fba3fd in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
> #12 0x in ?? ()
>
> On Fri, Oct 17, 2014 at 4:17 PM, Ben Pfaff wrote:
>
>> On Fri, Oct 17, 2014 at 04:07:25PM -0700, Madhu Challa wrote:
>> > dpif_flow_get initializes the flow_get part of the union, down the stack
>> > log_flow_message checks for actions || actions_len that could contain
>> > garbage leading to the crash.
>> >
>> > saw the crash once when running stress tests. can be easily recreated
>> > by running ovs-dpctl del-flows in a loop when traffic is going on
>> >
>> > Signed-off-by: Madhu Challa
>>
>> The actions aren't in the dpif_op so I don't see how this would help.
>> Can you explain?
>>
>> The actions are, instead, in the caller-provided dpif_flow. I guess
>> that the error is here in dpif_operate() where the code clears the
>> flow only after trying to log uninitialized garbage from it:
>> log_flow_get_message(dpif, get, error);
>>
>> if (error) {
>> memset(get->flow, 0, sizeof *get->flow);
>> }
>>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
