| Message ID | 1519214940-2982-1-git-send-email-i.maximets@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-upcall: Fix using uninitialized fitness. | expand |
On Wed, Feb 21, 2018 at 03:09:00PM +0300, Ilya Maximets wrote: > 'upcall_xlate()' makes a decision to compose slow path actions > by checking the 'upcall->fitness', which is not initialized in > case of calling from the 'upcall_cb()'. > > 'upcall_cb()' receives the real flow, so the fitness should be > initialized as perfect. > > Fixes following tests on travis: > > ofproto-dpif.at: ofproto-dpif megaflow - disabled - pmd > ofproto-dpif.at: ofproto-dpif - conntrack - output action > > CC: Ben Pfaff <blp@ovn.org> > Fixes: 687bafbb8a79 ("ofproto-dpif-upcall: Slow path flows that > datapath can't fully match.") > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> Thanks a lot for the fix. "struct upcall" is a large data structure (almost 2 kB on x86-32), so this one-line fix is going to be very expensive. To avoid a performance regression, I suggest using an assignment statement rather than an initializer, which necessarily initializes the entire structure. Would you mind respinning it that way? Thanks, Ben.
On 24.02.2018 01:13, Ben Pfaff wrote: > On Wed, Feb 21, 2018 at 03:09:00PM +0300, Ilya Maximets wrote: >> 'upcall_xlate()' makes a decision to compose slow path actions >> by checking the 'upcall->fitness', which is not initialized in >> case of calling from the 'upcall_cb()'. >> >> 'upcall_cb()' receives the real flow, so the fitness should be >> initialized as perfect. >> >> Fixes following tests on travis: >> >> ofproto-dpif.at: ofproto-dpif megaflow - disabled - pmd >> ofproto-dpif.at: ofproto-dpif - conntrack - output action >> >> CC: Ben Pfaff <blp@ovn.org> >> Fixes: 687bafbb8a79 ("ofproto-dpif-upcall: Slow path flows that >> datapath can't fully match.") >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > Thanks a lot for the fix. > > "struct upcall" is a large data structure (almost 2 kB on x86-32), so > this one-line fix is going to be very expensive. To avoid a performance > regression, I suggest using an assignment statement rather than an > initializer, which necessarily initializes the entire structure. Good point. Thanks. > > Would you mind respinning it that way? Sure, I'll send v2 with this change soon. Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 5eb20f7..ba712ca 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1254,7 +1254,9 @@ upcall_cb(const struct dp_packet *packet, const struct flow *flow, ovs_u128 *ufi { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); struct udpif *udpif = aux; - struct upcall upcall; + struct upcall upcall = { + .fitness = ODP_FIT_PERFECT, + }; bool megaflow; int error;
'upcall_xlate()' makes a decision to compose slow path actions by checking the 'upcall->fitness', which is not initialized in case of calling from the 'upcall_cb()'. 'upcall_cb()' receives the real flow, so the fitness should be initialized as perfect. Fixes following tests on travis: ofproto-dpif.at: ofproto-dpif megaflow - disabled - pmd ofproto-dpif.at: ofproto-dpif - conntrack - output action CC: Ben Pfaff <blp@ovn.org> Fixes: 687bafbb8a79 ("ofproto-dpif-upcall: Slow path flows that datapath can't fully match.") Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- ofproto/ofproto-dpif-upcall.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)