| Message ID | 20161206.135454.144280006.ken.ajiro@s1.itd.nes.nec.co.jp |
|---|---|
| State | Accepted |
| Headers | show |
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 <ken-ajiro@xr.jp.nec.com> > --- > 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.
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,
blp@ovn.org 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 <ken-ajiro@xr.jp.nec.com>
> > ---
> > 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.
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, > blp@ovn.org 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 <ken-ajiro@xr.jp.nec.com> > > > --- > > > 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.
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);
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 <ken-ajiro@xr.jp.nec.com> --- ofproto/connmgr.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)