Thanks for the clarification.  I agree that there is a bug in 2.5.1.  I
applied this patch to branch-2.5.

On Tue, Dec 06, 2016 at 05:38:31PM +0000, Ken Ajiro wrote:
> 
>   Ben,
> 
>   Sorry, it was my mistake.  I met this issue with OVS 2.5.1.
>   In OVS 2.5.1, packet_in scheduling is implemented as follows
>   in schedule_packet_in(),
> 
>     /* Make OFPT_PACKET_IN and hand over to packet scheduler. */
>     pinsched_send(ofconn->schedulers[pin.up.reason == OFPR_NO_MATCH ? 0 : 1],
>                   pin.up.flow_metadata.flow.in_port.ofp_port,
>                   ofputil_encode_packet_in(&pin.up,
>                                            ofconn_get_protocol(ofconn),
>                                            ofconn->packet_in_format),
>                   &txq);
>     do_send_packet_ins(ofconn, &txq);
> 
>   OVS2.5.1 treats schedulers[0] as "miss" and schedules[1] as "action", so
>   my patch will be needed.  However, as for master, my patch is not needed.
>   I'm sorry for the confusion.
> 
>   Thanks,
>   Ken
> 
>  On Tue, 6 Dec 2016 09:05:01 -0800,
>  [email protected] wrote
>  in E-mail "Re: [ovs-dev] [PATCH] ofproto: Incorrect statistics will be 
> increased":
> 
> > On Tue, Dec 06, 2016 at 04:55:11AM +0000, Ken Ajiro wrote:
> > > 
> > > When using controller packet-in rate limiting (controller_rate_limit in
> > > controller table), incorrect statistics will be increased. This patch
> > > fixes this issue.
> > > 
> > > Signed-off-by: Ken Ajiro <[email protected]>
> > > ---
> > >  ofproto/connmgr.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> > > index 4b927d6..bfd1ff5 100644
> > > --- a/ofproto/connmgr.c
> > > +++ b/ofproto/connmgr.c
> > > @@ -539,7 +539,7 @@ connmgr_get_controller_info(struct connmgr *mgr, 
> > > struct shash *info)
> > >  
> > >              for (i = 0; i < N_SCHEDULERS; i++) {
> > >                  if (ofconn->schedulers[i]) {
> > > -                    const char *name = i ? "miss" : "action";
> > > +                    const char *name = i == 0 ? "miss" : "action";
> > >                      struct pinsched_stats stats;
> > >  
> > >                      pinsched_get_stats(ofconn->schedulers[i], &stats);
> > 
> > Hmm, taking a closer look, now I'm confused a bit.
> > 
> > I only see two places where the elements of "schedulers" are
> > distinguished.  The first is in connmgr_send_async_msg(), which treats
> > schedulers[1] as "miss":
> > 
> >         bool is_miss = (am->pin.up.public.reason == OFPR_NO_MATCH ||
> >                         am->pin.up.public.reason == OFPR_EXPLICIT_MISS ||
> >                         am->pin.up.public.reason == OFPR_IMPLICIT_MISS);
> >         pinsched_send(ofconn->schedulers[is_miss],
> >                       am->pin.up.public.flow_metadata.flow.in_port.ofp_port,
> >                       msg, &txq);
> > 
> > So it seems that connmgr_get_controller_info() should also treat
> > schedulers[1] as "miss", but this patch appears to change it to do the
> > opposite.
> > 
> > Can you help me to understand?
> > 
> > Thanks,
> > 
> > Ben.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to