On Tue, Jul 26, 2016 at 11:20:38AM -0700, Daniele Di Proietto wrote:
> Hi Cascardo,
>
> thanks for your input on this. It's quite messy right now, but I believe
> we have a chance
> to fix this up.
>
> Replies inline
>
> 2016-07-26 7:33 GMT-07:00 Thadeu Lima de Souza Cascardo <casca...@redhat.com
> >:
>
> > On Mon, Jul 25, 2016 at 11:03:29AM -0700, Daniele Di Proietto wrote:
> > > 2016-07-25 9:57 GMT-07:00 Thadeu Lima de Souza Cascardo <
> > casca...@redhat.com
> > > >:
> > >
> > > > On Fri, Jul 22, 2016 at 02:49:39PM -0700, Daniele Di Proietto wrote:
> > > > > I would prefer if dpctl kept using the datapath types. The
> > translation
> > > > > from database types to datapath type should happen in ofproto, dpctl
> > is
> > > > > supposed to be used to interact with the datapath directly.
> > > > >
> > > > > What do you guys think?
> > > > >
> > > > > The rest of the series looks good to me as well.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Daniele
> > > > >
> > > >
> > > >
> > > Hi Cascardo,
> > >
> > > Thanks for the detailed analysis. The problem is that there are three
> > > types:
> > >
> > > a) the database type
> > > b) the port type in dpif-netdev
> > > c) the netdev type
> > >
> > > I was assuming that b and c are always equal, but they're not. The only
> > > case
> > > when they're not equal is the "ovs-netdev" (or "ovs-dummy") port.
> > >
> > > I think we can easily remove this case and make b and c always equal
> > > with the following changes:
> >
> > Well, we also have ofproto type.
> >
> >
> If I'm not mistaken, ofproto type is always equal to b) and therefore to c).
>
I didn't think so, I thought that a would equal the ofproto type. But it seems
you are right, and we can just have two types: database type and netdev type,
and make sure dpif and ofproto types match the netdev type.
>
> > I had a different approach, in which I would use the netdev_type when
> > doing the
> > query. That broke tests too. The affected tests were just dpctl output
> > shown to
> > the user. But I would expect some breakage when ofproto_query also used
> > the same
> > type and vswitchd would see the database type and ofproto type as
> > different and
> > try to reconfigure the port.
> >
> >
> I don't think so. In bridge_delete_or_reconfigure() we compare the ofproto
> type with
> iface->netdev_type:
>
> if (strcmp(ofproto_port.type, iface->netdev_type)
> || netdev_set_config(iface->netdev, &iface->cfg->options,
You are right. It was just some confusion because of the related problem I found
(and this code is a recent fix from myself because we were using the database
type).
The problem was that we were comparing the database type to "system" and my mind
was thinking "system" was the database type and the ofproto_port.type was
different. I just didn't look back at the code and made a quick assumption.
> NULL)) {
> /* The interface is the wrong type or can't be configured.
> * Delete it. */
> goto delete;
> }
>
>
> > Then, I looked at your patch below and noticed that you do the opposite,
> > you
> > eliminate the open_type and only use it for the internal type. Then I
> > thought
> > that would break other cases. But dpif_netdev_port_add uses the netdev_type
> > already. Hey...
> >
> > So, dpctl does see it as tap when I add an internal port. Which probably
> > means
> > ofproto_type is also tap. I guess we will have to fix that too.
> >
>
> To sum up, why do we have to fix that? The translation between the database
> type and the netdev type happens in vswitchd/bridge.c. The below layers,
> ofproto
> and dpif-netdev, deal with the netdev type directly.
>
> Is there a problem with this approach?
>
> The few changes I suggested fix the confusion for the "ovs-netdev" port.
>
I guess that just causes the slight difference in behavior that dpctl will
output "tap" instead of "internal" for the local port and only for the local
port. For other internal ports, "tap" was already used. As you mentioned dpctl
is a debugging tool and would be OK with that change, I will use your patch with
your from and my sign-off. Is that OK?
Thanks.
Cascardo.
>
> >
> > I am attaching my version of the patch here as well. Which of the 3
> > versions do
> > you think I should send? The original one I sent will not require changes
> > in the
> > tests and also doesn't change behavior for the user output, so I would
> > stick to
> > it for now.
> >
>
> Why is the code below necessary? ofproto already deals with the netdev
> type only, right?
> (maybe I'm missing something here, probably)
>
>
> >
> > Cascardo.
> >
> > ---8<---
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 37c2631..19b1f87 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -1437,11 +1437,12 @@ do_del_port(struct dp_netdev *dp, struct
> > dp_netdev_port *port)
> > }
> >
> > static void
> > -answer_port_query(const struct dp_netdev_port *port,
> > +answer_port_query(struct dp_netdev *dp OVS_UNUSED, const struct
> > dp_netdev_port *port,
> > struct dpif_port *dpif_port)
> > {
> > + const char *type = dpif_netdev_port_open_type(dp->class, port->type);
> > dpif_port->name = xstrdup(netdev_get_name(port->netdev));
> > - dpif_port->type = xstrdup(port->type);
> > + dpif_port->type = xstrdup(type);
> > dpif_port->port_no = port->port_no;
> > }
> >
> > @@ -1456,7 +1457,7 @@ dpif_netdev_port_query_by_number(const struct dpif
> > *dpif, odp_port_t port_no,
> > ovs_mutex_lock(&dp->port_mutex);
> > error = get_port_by_number(dp, port_no, &port);
> > if (!error && dpif_port) {
> > - answer_port_query(port, dpif_port);
> > + answer_port_query(dp, port, dpif_port);
> > }
> > ovs_mutex_unlock(&dp->port_mutex);
> >
> > @@ -1474,7 +1475,7 @@ dpif_netdev_port_query_by_name(const struct dpif
> > *dpif, const char *devname,
> > ovs_mutex_lock(&dp->port_mutex);
> > error = get_port_by_name(dp, devname, &port);
> > if (!error && dpif_port) {
> > - answer_port_query(port, dpif_port);
> > + answer_port_query(dp, port, dpif_port);
> > }
> > ovs_mutex_unlock(&dp->port_mutex);
> >
> > ---8<---
> >
> > >
> > > ---8<---
> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > index 787851d..effa7e0 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -941,7 +941,9 @@ create_dp_netdev(const char *name, const struct
> > > dpif_class *class,
> > > ovs_mutex_lock(&dp->port_mutex);
> > > dp_netdev_set_nonpmd(dp);
> > >
> > > - error = do_add_port(dp, name, "internal", ODPP_LOCAL);
> > > + error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class,
> > > +
> > "internal"),
> > > + ODPP_LOCAL);
> > > ovs_mutex_unlock(&dp->port_mutex);
> > > if (error) {
> > > dp_netdev_free(dp);
> > > @@ -1129,7 +1131,7 @@ hash_port_no(odp_port_t port_no)
> > > }
> > >
> > > static int
> > > -port_create(const char *devname, const char *open_type, const char
> > *type,
> > > +port_create(const char *devname, const char *type,
> > > odp_port_t port_no, struct dp_netdev_port **portp)
> > > {
> > > struct netdev_saved_flags *sf;
> > > @@ -1142,7 +1144,7 @@ port_create(const char *devname, const char
> > > *open_type, const char *type,
> > > *portp = NULL;
> > >
> > > /* Open and validate network device. */
> > > - error = netdev_open(devname, open_type, &netdev);
> > > + error = netdev_open(devname, type, &netdev);
> > > if (error) {
> > > return error;
> > > }
> > > @@ -1233,8 +1235,7 @@ do_add_port(struct dp_netdev *dp, const char
> > > *devname, const char *type,
> > > return EEXIST;
> > > }
> > >
> > > - error = port_create(devname, dpif_netdev_port_open_type(dp->class,
> > > type),
> > > - type, port_no, &port);
> > > + error = port_create(devname, type, port_no, &port);
> > > if (error) {
> > > return error;
> > > }
> > > root@diproiettod-dev:~/ovs# git diff
> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > index 787851d..effa7e0 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -941,7 +941,9 @@ create_dp_netdev(const char *name, const struct
> > > dpif_class *class,
> > > ovs_mutex_lock(&dp->port_mutex);
> > > dp_netdev_set_nonpmd(dp);
> > >
> > > - error = do_add_port(dp, name, "internal", ODPP_LOCAL);
> > > + error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class,
> > > +
> > "internal"),
> > > + ODPP_LOCAL);
> > > ovs_mutex_unlock(&dp->port_mutex);
> > > if (error) {
> > > dp_netdev_free(dp);
> > > @@ -1129,7 +1131,7 @@ hash_port_no(odp_port_t port_no)
> > > }
> > >
> > > static int
> > > -port_create(const char *devname, const char *open_type, const char
> > *type,
> > > +port_create(const char *devname, const char *type,
> > > odp_port_t port_no, struct dp_netdev_port **portp)
> > > {
> > > struct netdev_saved_flags *sf;
> > > @@ -1142,7 +1144,7 @@ port_create(const char *devname, const char
> > > *open_type, const char *type,
> > > *portp = NULL;
> > >
> > > /* Open and validate network device. */
> > > - error = netdev_open(devname, open_type, &netdev);
> > > + error = netdev_open(devname, type, &netdev);
> > > if (error) {
> > > return error;
> > > }
> > > @@ -1233,8 +1235,7 @@ do_add_port(struct dp_netdev *dp, const char
> > > *devname, const char *type,
> > > return EEXIST;
> > > }
> > >
> > > - error = port_create(devname, dpif_netdev_port_open_type(dp->class,
> > > type),
> > > - type, port_no, &port);
> > > + error = port_create(devname, type, port_no, &port);
> > > if (error) {
> > > return error;
> > > }
> > > ---8<---
> > >
> > > With this incremental case 2 is covered, dpctl/show always shows the
> > > datapath type.
> > > (The incremental also requires some testsuite changes)
> > >
> > > For case 1 and 3 is just a matter of deciding if we want to support
> > database
> > > types (in addition to netdev) in dpctl. I would lean towards no: I find
> > it
> > > confusing
> > > that both types are acceptable.
> > >
> > > That said, I feel like I'm nitpicking, dpctl is a tool used for debugging
> > > and I think
> > > we should do whatever comes easier.
> > >
> > > Thanks,
> > >
> > > Daniele
> > >
> > > Hi, Daniele.
> > > >
> > > > Thanks for the comment.
> > > >
> > > > The best example that breaks currently is the internal type on the
> > netdev
> > > > datapath.
> > > >
> > > > There are three scenarios in dpctl, and two of them are changed with
> > this
> > > > patch.
> > > >
> > > > 1) The user input type, given for add-if. In this particular case, the
> > code
> > > > would try to use the "internal" type, but thinks would break as the
> > Linux
> > > > internal type would not work for the netdev datapath. The user was
> > > > expected to
> > > > use "tap" instead. We the patch applied, either way should work. So we
> > are
> > > > not
> > > > breaking behavior.
> > > >
> > > > 2) When the type is output to the user. The current patch does not
> > change
> > > > its
> > > > behavior, which basically prints what we get from dpif_port_query.
> > However,
> > > > dpif-netdev uses a cache of the database type and returns that, not the
> > > > netdev
> > > > type. That could be changed, but then the output to the user would be
> > the
> > > > real
> > > > dpif. Would that be acceptable? It could break scripts out there which
> > > > look for
> > > > internal. Or would dpctl do the reverse mapping? That would require
> > some
> > > > new
> > > > code on the dpif level.
> > > >
> > >
> > > > 3) Using the dpif_port.type in netdev_open. We could use the same
> > > > alternative
> > > > solution here as proposed for scenario 2. Make dpif_port_query return
> > the
> > > > real
> > > > netdev type. This would require the new reverse mapping in order to be
> > > > used also
> > > > at the ofproto level, so ofproto_port_query uses the database type.
> > > >
> > > > I have a patch that changes dpif-netdev to return the netdev open type.
> > > > However,
> > > > as it broke dpctl output, I decided to provice this current patch
> > instead,
> > > > as I
> > > > realized a lot more would be needed on other code, and we risked
> > breaking
> > > > a lot
> > > > of behavior.
> > > >
> > > > I would keep this patch as is in the series, as it keeps behavior, and
> > > > improves
> > > > the case in add-if, without breaking it; and leave the dpif_port_query
> > fix
> > > > and
> > > > reverse mapping to another day.
> > > >
> > > > What do you think?
> > > >
> > > > Regards.
> > > > Cascardo.
> > > >
> > > >
> > > > > 2016-07-18 10:00 GMT-07:00 Thadeu Lima de Souza Cascardo <
> > > > > casca...@redhat.com>:
> > > > >
> > > > > > dpctl uses a user or database defined type when calling
> > netdev_open.
> > > > > > Instead, it
> > > > > > should use the type from dpif_port_open_type. Otherwise, when
> > using the
> > > > > > internal
> > > > > > type, it could open it with that type instead of the correct one,
> > which
> > > > > > would be
> > > > > > tap or dummy.
> > > > > > ---
> > > > > > lib/dpctl.c | 17 ++++++++++++-----
> > > > > > 1 file changed, 12 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > > > > > index 003602a..f896161 100644
> > > > > > --- a/lib/dpctl.c
> > > > > > +++ b/lib/dpctl.c
> > > > > > @@ -274,7 +274,8 @@ dpctl_add_if(int argc OVS_UNUSED, const char
> > > > *argv[],
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > - error = netdev_open(name, type, &netdev);
> > > > > > + error = netdev_open(name,
> > dpif_port_open_type(dpif_type(dpif),
> > > > > > type),
> > > > > > + &netdev);
> > > > > > if (error) {
> > > > > > dpctl_error(dpctl_p, error, "%s: failed to open
> > network
> > > > > > device",
> > > > > > name);
> > > > > > @@ -356,7 +357,8 @@ dpctl_set_if(int argc, const char *argv[],
> > struct
> > > > > > dpctl_params *dpctl_p)
> > > > > > dpif_port_destroy(&dpif_port);
> > > > > >
> > > > > > /* Retrieve its existing configuration. */
> > > > > > - error = netdev_open(name, type, &netdev);
> > > > > > + error = netdev_open(name,
> > dpif_port_open_type(dpif_type(dpif),
> > > > > > type),
> > > > > > + &netdev);
> > > > > > if (error) {
> > > > > > dpctl_error(dpctl_p, error, "%s: failed to open
> > network
> > > > > > device",
> > > > > > name);
> > > > > > @@ -558,10 +560,13 @@ show_dpif(struct dpif *dpif, struct
> > dpctl_params
> > > > > > *dpctl_p)
> > > > > > qsort(port_nos, n_port_nos, sizeof *port_nos,
> > compare_port_nos);
> > > > > >
> > > > > > for (int i = 0; i < n_port_nos; i++) {
> > > > > > + const char *type;
> > > > > > if (dpif_port_query_by_number(dpif, port_nos[i],
> > &dpif_port))
> > > > {
> > > > > > continue;
> > > > > > }
> > > > > >
> > > > > > + type = dpif_port_open_type(dpif_type(dpif),
> > dpif_port.type);
> > > > > > +
> > > > > > dpctl_print(dpctl_p, "\tport %u: %s",
> > > > > > dpif_port.port_no, dpif_port.name);
> > > > > >
> > > > > > @@ -570,7 +575,7 @@ show_dpif(struct dpif *dpif, struct
> > dpctl_params
> > > > > > *dpctl_p)
> > > > > >
> > > > > > dpctl_print(dpctl_p, " (%s", dpif_port.type);
> > > > > >
> > > > > > - error = netdev_open(dpif_port.name, dpif_port.type,
> > > > &netdev);
> > > > > > + error = netdev_open(dpif_port.name, type, &netdev);
> > > > > > if (!error) {
> > > > > > struct smap config;
> > > > > >
> > > > > > @@ -603,7 +608,7 @@ show_dpif(struct dpif *dpif, struct
> > dpctl_params
> > > > > > *dpctl_p)
> > > > > > struct netdev_stats s;
> > > > > > int error;
> > > > > >
> > > > > > - error = netdev_open(dpif_port.name, dpif_port.type,
> > > > &netdev);
> > > > > > + error = netdev_open(dpif_port.name, type, &netdev);
> > > > > > if (error) {
> > > > > > dpctl_print(dpctl_p, ", open failed (%s)",
> > > > > > ovs_strerror(error));
> > > > > > @@ -891,6 +896,7 @@ get_in_port_netdev_from_key(struct dpif *dpif,
> > > > const
> > > > > > struct ofpbuf *key)
> > > > > > struct dpif_port dpif_port;
> > > > > > odp_port_t port_no;
> > > > > > int error;
> > > > > > + const char *type;
> > > > > >
> > > > > > port_no = ODP_PORT_C(nl_attr_get_u32(in_port_nla));
> > > > > > error = dpif_port_query_by_number(dpif, port_no,
> > &dpif_port);
> > > > > > @@ -898,7 +904,8 @@ get_in_port_netdev_from_key(struct dpif *dpif,
> > > > const
> > > > > > struct ofpbuf *key)
> > > > > > goto out;
> > > > > > }
> > > > > >
> > > > > > - netdev_open(dpif_port.name, dpif_port.type, &dev);
> > > > > > + type = dpif_port_open_type(dpif_type(dpif),
> > dpif_port.type);
> > > > > > + netdev_open(dpif_port.name, type, &dev);
> > > > > > dpif_port_destroy(&dpif_port);
> > > > > > }
> > > > > >
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > > > > _______________________________________________
> > > > > > dev mailing list
> > > > > > dev@openvswitch.org
> > > > > > http://openvswitch.org/mailman/listinfo/dev
> > > > > >
> > > > > _______________________________________________
> > > > > dev mailing list
> > > > > dev@openvswitch.org
> > > > > http://openvswitch.org/mailman/listinfo/dev
> > > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
