Re: [ovs-dev] [RFC v4 1/1] datapath: Add a new action check_pkt_len
On Sat, Feb 23, 2019 at 2:35 AM Gregory Rose wrote:
> Numan,
>
> I intend to test and review this on my local net-next branch but I'm not
> feeling well so it will probably be
> early next week before I can do that. Sorry for the delay...
>
>
No worries. Please take your time.
If you want to test it out, you probably need the corresponding
ovs-vswitchd patch.
I still need to address the review comments from Ben. But you can use the
one from here -
https://github.com/numansiddique/ovs/tree/ovn_mtu_fix/rfc_v4_p4
https://github.com/numansiddique/ovs/commits/ovn_mtu_fix/rfc_v4_p4 for your
testing.
Thanks
Numan
- Greg
>
> On 2/21/2019 10:42 AM, nusid...@redhat.com wrote:
> > From: Numan Siddique
> >
> > [Please note, this patch is submitted as RFC in ovs-dev ML to
> > get feedback before submitting to netdev ML. You need net-next tree
> > to apply this patch]
> >
> > This patch adds a new action - 'check_pkt_len' which checks the
> > packet length and executes a set of actions if the packet
> > length is greater than the specified length or executes
> > another set of actions if the packet length is lesser or equal to.
> >
> > This action takes below nlattrs
> >* OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
> >
> >* OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER - Nested actions
> > to apply if the packet length is greater than the specified
> 'pkt_len'
> >
> >* OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested
> > actions to apply if the packet length is lesser or equal to the
> > specified 'pkt_len'.
> >
> > The main use case for adding this action is to solve the packet
> > drops because of MTU mismatch in OVN virtual networking solution.
> > When a VM (which belongs to a logical switch of OVN) sends a packet
> > destined to go via the gateway router and if the nic which provides
> > external connectivity, has a lesser MTU, OVS drops the packet
> > if the packet length is greater than this MTU.
> >
> > With the help of this action, OVN will check the packet length
> > and if it is greater than the MTU size, it will generate an
> > ICMP packet (type 3, code 4) and includes the next hop mtu in it
> > so that the sender can fragment the packets.
> >
> > Reported-at:
> > https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
> > Suggested-by: Ben Pfaff
> > Signed-off-by: Numan Siddique
> > CC: Ben Pfaff
> > CC: Greg Rose
> > CC: Lorenzo Bianconi
> > ---
> >
> > v3 -> v4
> >
> > * v4 only has 1 patch - datapath patch which implements the
> > * check_pkt_len action
> > * Addressed the review comments from Lorenzo, Ben and Greg
> >
> >
> > include/uapi/linux/openvswitch.h | 42
> > net/openvswitch/actions.c| 49 +
> > net/openvswitch/flow_netlink.c | 171 +++
> > 3 files changed, 262 insertions(+)
> >
> > diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> > index dbe0cbe4f1b7..05ab885c718d 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -798,6 +798,44 @@ struct ovs_action_push_eth {
> > struct ovs_key_ethernet addresses;
> > };
> >
> > +/*
> > + * enum ovs_check_pkt_len_attr - Attributes for
> %OVS_ACTION_ATTR_CHECK_PKT_LEN.
> > + *
> > + * @OVS_CHECK_PKT_LEN_ATTR_PKT_LEN: u16 Packet length to check for.
> > + * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER: Nested OVS_ACTION_ATTR_*
> > + * actions to apply if the packer length is greater than the specified
> > + * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
> > + * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested
> OVS_ACTION_ATTR_*
> > + * actions to apply if the packer length is lesser or equal to the
> specified
> > + * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
> > + */
> > +enum ovs_check_pkt_len_attr {
> > + OVS_CHECK_PKT_LEN_ATTR_UNSPEC,
> > + OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
> > + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
> > + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
> > + __OVS_CHECK_PKT_LEN_ATTR_MAX,
> > +
> > +#ifdef __KERNEL__
> > + OVS_CHECK_PKT_LEN_ATTR_ARG /* struct check_pkt_len_arg */
> > +#endif
> > +};
> > +
> > +#define OVS_CHECK_PKT_LEN_ATTR_MAX (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)
> > +
> > +#ifdef __KERNEL__
> > +struct check_pkt_len_arg {
> > + u16 pkt_len;/* Same value as OVS_CHECK_PKT_LEN_ATTR_PKT_LEN'.
> */
> > + bool exec_for_greater; /* When true, actions in IF_GREATE will
> > + * not change flow keys. False otherwise.
> > + */
> > + bool exec_for_lesser_equal; /* When true, actions in IF_LESS_EQUAL
> > + * will not change flow keys. False
> > + * otherwise.
> > + */
> > +};
> > +#endif
> > +
> > /**
> >* enum ovs_action_attr - Action types.
> >*
> > @@ -842,6 +
Re: [ovs-dev] Avoid packet drop on LACP bond after link up
Bleep bloop. Greetings Nitin Katiyar, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. build: /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/jhash.lo -MD -MP -MF $depbase.Tpo -c -o lib/jhash.lo lib/jhash.c &&\ mv -f $depbase.Tpo $depbase.Plo libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/jhash.lo -MD -MP -MF lib/.deps/jhash.Tpo -c lib/jhash.c -o lib/jhash.o depbase=`echo lib/json.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\ /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/json.lo -MD -MP -MF $depbase.Tpo -c -o lib/json.lo lib/json.c &&\ mv -f $depbase.Tpo $depbase.Plo libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/json.lo -MD -MP -MF lib/.deps/json.Tpo -c lib/json.c -o lib/json.o depbase=`echo lib/jsonrpc.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\ /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/jsonrpc.lo -MD -MP -MF $depbase.Tpo -c -o lib/jsonrpc.lo lib/jsonrpc.c &&\ mv -f $depbase.Tpo $depbase.Plo libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/jsonrpc.lo -MD -MP -MF lib/.deps/jsonrpc.Tpo -c lib/jsonrpc.c -o lib/jsonrpc.o depbase=`echo lib/lacp.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\ /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/lacp.lo -MD -MP -MF $depbase.Tpo -c -o lib/lacp.lo lib/lacp.c &&\ mv -f $depbase.Tpo $depbase.Plo libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/lacp.lo -MD -MP -MF lib/.deps/lacp.Tpo -c lib/lacp.c -o lib/lacp.o lib/lacp.c: In function 'lacp_process_packet': lib/lacp.c:329:52: error: unused parameter 'bond' [-Werror=unused-parameter] lacp_process_packet(struct lacp *lacp, const void *bond, const void *slave_, ^ cc1: all warnings being treated as errors make[2]: *** [lib/lacp.lo] Error 1 make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace' make[1
[ovs-dev] [PATCH v2 2/2] Avoid packet drop on LACP bond after link up
Problem:
On certain Fortville NICs it has been observed that PHY UP detection can
get delayed (sometimes up to 4-5 secs). When the driver fails to fetch
PHY status as UP even though its actually UP, LACP packets can get
exchanged and LACP slave brought UP. In such a case, the remote end
would start sending traffic on that slave, but OVS drops it, as the
bond-slave is not yet enabled due to PHY DOWN.
Fix:
The main intention here is delay LACP negotiation until carrier (PHY)
status is read as UP.
1. In port_run()/bundle_run(), cache the carrier status in
"struct ofport_dpif"/"struct bond_slave".
2. When carrier state is DOWN, do not send any LACPDUs and
drop any received LACPDUs.
3. When LACP state changes or LACPDU is received, trigger re-checking
of carrier-state (in port_run()) by incrementing the connectivity
sequence number to find out the true carrier state as fast as
possible.
Signed-off-by: Nitin Katiyar
Signed-off-by: Manohar Krishnappa Chidambaraswamy
Co-authored-by: Manohar Krishnappa Chidambaraswamy
---
lib/lacp.c | 40 +---
lib/lacp.h | 4 ++--
ofproto/bond.c | 30 ++
ofproto/bond.h | 3 +++
ofproto/ofproto-dpif.c | 10 +++---
tests/ofproto-dpif.at | 3 +++
6 files changed, 78 insertions(+), 12 deletions(-)
diff --git a/lib/lacp.c b/lib/lacp.c
index 7ac85f9..359b6ce 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -33,6 +33,7 @@
#include "unixctl.h"
#include "openvswitch/vlog.h"
#include "util.h"
+#include "ofproto/bond.h"
VLOG_DEFINE_THIS_MODULE(lacp);
@@ -148,7 +149,7 @@ static void slave_get_actor(struct slave *, struct
lacp_info *actor)
OVS_REQUIRES(mutex);
static void slave_get_priority(struct slave *, struct lacp_info *priority)
OVS_REQUIRES(mutex);
-static bool slave_may_tx(const struct slave *)
+static bool slave_may_tx(const void *bond, const struct slave *)
OVS_REQUIRES(mutex);
static struct slave *slave_lookup(const struct lacp *, const void *slave)
OVS_REQUIRES(mutex);
@@ -334,6 +335,7 @@ lacp_process_packet(struct lacp *lacp, const void *bond,
const void *slave_,
const struct lacp_pdu *pdu;
long long int tx_rate;
struct slave *slave;
+bool carrier_up = false;
bool lacp_may_enable = false;
lacp_lock();
@@ -350,6 +352,21 @@ lacp_process_packet(struct lacp *lacp, const void *bond,
const void *slave_,
goto out;
}
+/*
+ * On some NICs L1 state reporting is slow. In case LACP packets are
+ * received while carrier (L1) state is still down, drop the LACPDU and
+ * trigger re-checking of L1 state.
+ */
+carrier_up = bond_slave_get_carrier(bond, slave_);
+if (!carrier_up) {
+seq_change(connectivity_seq_get());
+
+VLOG_INFO_RL(&rl, "carrier still DOWN - conn seq changed for %s, "
+ "dropping packet\n", slave->name);
+
+goto out;
+}
+
slave->status = LACP_CURRENT;
tx_rate = lacp->fast ? LACP_FAST_TIME_TX : LACP_SLOW_TIME_TX;
timer_set_duration(&slave->rx, LACP_RX_MULTIPLIER * tx_rate);
@@ -529,7 +546,8 @@ lacp_slave_is_current(const struct lacp *lacp, const void
*slave_)
/* This function should be called periodically to update 'lacp'. */
void
-lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) OVS_EXCLUDED(mutex)
+lacp_run(struct lacp *lacp, const void *bond,
+ lacp_send_pdu *send_pdu) OVS_EXCLUDED(mutex)
{
struct slave *slave;
@@ -559,7 +577,7 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu)
OVS_EXCLUDED(mutex)
HMAP_FOR_EACH (slave, node, &lacp->slaves) {
struct lacp_info actor;
-if (!slave_may_tx(slave)) {
+if (!slave_may_tx(bond, slave)) {
continue;
}
@@ -588,13 +606,13 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu)
OVS_EXCLUDED(mutex)
/* Causes poll_block() to wake up when lacp_run() needs to be called again. */
void
-lacp_wait(struct lacp *lacp) OVS_EXCLUDED(mutex)
+lacp_wait(struct lacp *lacp, const void *bond) OVS_EXCLUDED(mutex)
{
struct slave *slave;
lacp_lock();
HMAP_FOR_EACH (slave, node, &lacp->slaves) {
-if (slave_may_tx(slave)) {
+if (slave_may_tx(bond, slave)) {
timer_wait(&slave->tx);
}
@@ -818,9 +836,17 @@ slave_get_priority(struct slave *slave, struct lacp_info
*priority)
}
static bool
-slave_may_tx(const struct slave *slave) OVS_REQUIRES(mutex)
+slave_may_tx(const void *bond, const struct slave *slave)
+OVS_REQUIRES(mutex)
{
-return slave->lacp->active || slave->status != LACP_DEFAULTED;
+bool carrier_up = true;
+
+if (bond) {
+carrier_up = bond_slave_get_carrier(bond, slave->aux);
+}
+
+return carrier_up &&
+(slave->lacp->active || slave->status != LACP_DEFAULTED);
}
static struct slave *
diff --git a/lib/lacp.h b/lib/lacp.h
index 1505c2c..9a302c9 100644
--- a/lib/
[ovs-dev] [PATCH v2 1/2] Avoid packet drop on LACP bond after link up
Problem:
During port DOWN->UP of link (slave) in a LACP bond, after receiving the
LACPDU with SYNC set for both actor and partner, the bond-slave remains
"disabled" until OVS main thread runs LACP state machine and eventually
"enables" the bond-slave. With this, we have observed delays in the order
of 350ms and packets are dropped in OVS due to bond-admissibility check
(packets received on slave in "disabled" state are dropped).
Fix:
When a LACPDU is received, evaluate whether LACP slave can be enabled
(slave_may_enable()) and set LACP slave's may_enable from the datapath
thread itself. When may_enable = TRUE, it means L1 state is UP and
LACP-SYNC is done and it is waiting for the main thread to enable the
slave. Relax the check in bond_check_admissibility() to check for both
"enable" and "may_enable" of the LACP slave. This would avoid dropping
of packets until the main thread enables the slave from bundle_run().
Signed-off-by: Nitin Katiyar
Signed-off-by: Manohar Krishnappa Chidambaraswamy
Co-authored-by: Manohar Krishnappa Chidambaraswamy
---
lib/lacp.c | 12 ++--
lib/lacp.h | 3 ++-
ofproto/bond.c | 16 +---
ofproto/ofproto-dpif-xlate.c | 11 ++-
4 files changed, 35 insertions(+), 7 deletions(-)
diff --git a/lib/lacp.c b/lib/lacp.c
index d6b36aa..7ac85f9 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -154,6 +154,7 @@ static struct slave *slave_lookup(const struct lacp *,
const void *slave)
OVS_REQUIRES(mutex);
static bool info_tx_equal(struct lacp_info *, struct lacp_info *)
OVS_REQUIRES(mutex);
+static bool slave_may_enable__(struct slave *slave) OVS_REQUIRES(mutex);
static unixctl_cb_func lacp_unixctl_show;
static unixctl_cb_func lacp_unixctl_show_stats;
@@ -324,8 +325,8 @@ lacp_is_active(const struct lacp *lacp) OVS_EXCLUDED(mutex)
/* Processes 'packet' which was received on 'slave_'. This function should be
* called on all packets received on 'slave_' with Ethernet Type ETH_TYPE_LACP.
*/
-void
-lacp_process_packet(struct lacp *lacp, const void *slave_,
+bool
+lacp_process_packet(struct lacp *lacp, const void *bond, const void *slave_,
const struct dp_packet *packet)
OVS_EXCLUDED(mutex)
{
@@ -333,6 +334,7 @@ lacp_process_packet(struct lacp *lacp, const void *slave_,
const struct lacp_pdu *pdu;
long long int tx_rate;
struct slave *slave;
+bool lacp_may_enable = false;
lacp_lock();
slave = slave_lookup(lacp, slave_);
@@ -362,8 +364,14 @@ lacp_process_packet(struct lacp *lacp, const void *slave_,
slave->partner = pdu->actor;
}
+/* Evaluate may_enable here to avoid dropping of packets till main thread
+ * sets may_enable to true. */
+lacp_may_enable = slave_may_enable__(slave);
+
out:
lacp_unlock();
+
+return lacp_may_enable;
}
/* Returns the lacp_status of the given 'lacp' object (which may be NULL). */
diff --git a/lib/lacp.h b/lib/lacp.h
index f35cff5..1505c2c 100644
--- a/lib/lacp.h
+++ b/lib/lacp.h
@@ -46,7 +46,8 @@ struct lacp *lacp_ref(const struct lacp *);
void lacp_configure(struct lacp *, const struct lacp_settings *);
bool lacp_is_active(const struct lacp *);
-void lacp_process_packet(struct lacp *, const void *slave,
+bool lacp_process_packet(struct lacp *, const void *bond,
+ const void *slave,
const struct dp_packet *packet);
enum lacp_status lacp_status(const struct lacp *);
diff --git a/ofproto/bond.c b/ofproto/bond.c
index d2a8b1f..cd6fd33 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -794,6 +794,7 @@ bond_check_admissibility(struct bond *bond, const void
*slave_,
{
enum bond_verdict verdict = BV_DROP;
struct bond_slave *slave;
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
ovs_rwlock_rdlock(&rwlock);
slave = bond_slave_lookup(bond, slave_);
@@ -811,7 +812,11 @@ bond_check_admissibility(struct bond *bond, const void
*slave_,
* drop all incoming traffic except if lacp_fallback_ab is enabled. */
switch (bond->lacp_status) {
case LACP_NEGOTIATED:
-verdict = slave->enabled ? BV_ACCEPT : BV_DROP;
+/* To reduce packet-drops due to delay in enabling of slave (post
+ * LACP-SYNC), from main thread, check for may_enable as well.
+ * When may_enable is TRUE, it means LACP is UP and waiting for the
+ * main thread to run LACP state machine and enable the slave. */
+verdict = (slave->enabled || slave->may_enable) ? BV_ACCEPT : BV_DROP;
goto out;
case LACP_CONFIGURED:
if (!bond->lacp_fallback_ab) {
@@ -847,8 +852,6 @@ bond_check_admissibility(struct bond *bond, const void
*slave_,
/* Drop all packets which arrive on backup slaves. This is similar to
* how Linux bonding handles active-backup bonds. */
if (bond->active_slave != slave) {
-static struct
[ovs-dev] Hongkong or china representative needed urgently (job offer)
I am the owner of Hassan Jamel Industries ltd. and it is a firm based in Morocco in Africa. Our company have problems collecting money for the already supplied works because the international USD currency is high in Morocco and when we receive USD payment to our bank account, we get payed by our local currency which is devalued by our bank. This becomes a great loss for us and we loose alot of money almost in every business we had We need your help to receive payments from our customers which will be transferred to your Bank Account. After cashing the fund, then we would inform you on how to resend the rest of the money to us. As your income, you will be getting 5% of any amount of USD you receive and process for us. This will save alot of money for us if we can receive USD not in Morocco. If you are interested in our offer, please reply to me to continue further:. Best Regards Hassan Jamel Managing Director Hassan Jamel Industries ltd Email: hassanjamel0...@gmail.com ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V5 0/2] Do not rewrite fields with the same values as matched
On 2/22/2019 6:42 PM, Ben Pfaff wrote:
> On Sun, Feb 17, 2019 at 09:18:46AM +, Eli Britstein wrote:
>> This patch set avoids unnecessary rewrite actions to fields with the
>> same values as matched on.
>>
>> Patch 1 is a pre-step of generating ovs key fields macros
>> Patch 2 avoids the unnecessary rewrites and adapts the tests accordingly
> Thanks for the revision.
>
> Do you foresee other uses of OVS_KEY_FIELD in the future? As is,
> there's a lot of duplication here from the numerous declarations like
>
> struct ovs_key_field_properties ovs_key_nd_extensions_properties[] = {
> #define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_nd_extensions,
> name), sizeof(type)},
> OVS_KEY_ND_EXTENSIONS_FIELDS
> {0, 0}
> #undef OVS_KEY_FIELD
> };
>
> If this is the only currently foreseen use, it would be better to have
> the code generator just generate the declarations directly instead of
> forcing these later duplications.
Please note the definitions are not exactly duplicated - each define
refers to a different struct. Currently I don't know what other uses
will be needed/done in the future. However, I think we should remain
with more generic and flexible code, that will allow easier use of those
macros in the future rather than specific macros.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
