| Message ID | 57976371.30904@samsung.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On 26/07/2016 06:19, "Ilya Maximets" <i.maximets@samsung.com> wrote: >On 26.07.2016 04:45, Daniele Di Proietto wrote: >> Thanks for the patch >> >> It looks good to me, a few minor comments inline >> >> >> On 15/07/2016 04:54, "Ilya Maximets" <i.maximets@samsung.com> wrote: >> >>> This commit adds functionality to pass value of 'other_config' column >>> of 'Interface' table to datapath. >>> >>> This may be used to pass not directly connected with netdev options and >>> configure behaviour of the datapath for different ports. >>> For example: pinning of rx queues to polling threads in dpif-netdev. >>> >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>> --- >>> lib/dpif-netdev.c | 1 + >>> lib/dpif-netlink.c | 1 + >>> lib/dpif-provider.h | 5 +++++ >>> lib/dpif.c | 17 +++++++++++++++++ >>> lib/dpif.h | 1 + >>> ofproto/ofproto-dpif.c | 16 ++++++++++++++++ >>> ofproto/ofproto-provider.h | 5 +++++ >>> ofproto/ofproto.c | 29 +++++++++++++++++++++++++++++ >>> ofproto/ofproto.h | 2 ++ >>> vswitchd/bridge.c | 2 ++ >>> 10 files changed, 79 insertions(+) >>> >>> [...] >>> >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>> index ce9383a..97510a9 100644 >>> --- a/ofproto/ofproto-dpif.c >>> +++ b/ofproto/ofproto-dpif.c >>> @@ -3542,6 +3542,21 @@ port_del(struct ofproto *ofproto_, ofp_port_t ofp_port) >>> } >>> >>> static int >>> +port_set_config(struct ofproto *ofproto_, ofp_port_t ofp_port, >>> + const struct smap *cfg) >> >> Can we change this to directly take struct ofport_dpif *ofport instead of ofp_port_t? > >We can't get 'struct ofport_dpif *' because ofproto layer knows nothing >about 'ofport_dpif' structure. All that we can is to get 'struct ofport *' >and cast it. You're right, using 'struct ofport *' seems better, thanks. With the below diff Acked-by: Daniele Di Proietto <diproiettod@vmware.com> > >How about following fixup to this patch: >---------------------------------------------------------------------- >diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >index 3a13326..79f2aa0 100644 >--- a/ofproto/ofproto-dpif.c >+++ b/ofproto/ofproto-dpif.c >@@ -3543,14 +3543,13 @@ port_del(struct ofproto *ofproto_, ofp_port_t ofp_port) > } > > static int >-port_set_config(struct ofproto *ofproto_, ofp_port_t ofp_port, >- const struct smap *cfg) >+port_set_config(const struct ofport *ofport_, const struct smap *cfg) > { >- struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); >- struct ofport_dpif *ofport = ofp_port_to_ofport(ofproto, ofp_port); >+ struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); >+ struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto); > >- if (!ofport || sset_contains(&ofproto->ghost_ports, >- netdev_get_name(ofport->up.netdev))) { >+ if (sset_contains(&ofproto->ghost_ports, >+ netdev_get_name(ofport->up.netdev))) { > return 0; > } > >diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >index 2fc7452..7156814 100644 >--- a/ofproto/ofproto-provider.h >+++ b/ofproto/ofproto-provider.h >@@ -972,10 +972,9 @@ struct ofproto_class { > * convenient. */ > int (*port_del)(struct ofproto *ofproto, ofp_port_t ofp_port); > >- /* Refreshes dtapath configuration of port number 'ofp_port' in 'ofproto'. >+ /* Refreshes datapath configuration of 'port'. > * Returns 0 if successful, otherwise a positive errno value. */ >- int (*port_set_config)(struct ofproto *ofproto, ofp_port_t ofp_port, >- const struct smap *cfg); >+ int (*port_set_config)(const struct ofport *port, const struct smap *cfg); > > /* Get port stats */ > int (*port_get_stats)(const struct ofport *port, >diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >index c66c866..6cd2600 100644 >--- a/ofproto/ofproto.c >+++ b/ofproto/ofproto.c >@@ -2079,7 +2079,7 @@ ofproto_port_del(struct ofproto *ofproto, ofp_port_t ofp_port) > return error; > } > >-/* Refreshes dtapath configuration of port number 'ofp_port' in 'ofproto'. >+/* Refreshes datapath configuration of port number 'ofp_port' in 'ofproto'. > * > * This function has no effect if 'ofproto' does not have a port 'ofp_port'. */ > void >@@ -2097,10 +2097,10 @@ ofproto_port_set_config(struct ofproto *ofproto, ofp_port_t ofp_port, > } > > error = (ofproto->ofproto_class->port_set_config >- ? ofproto->ofproto_class->port_set_config(ofproto, ofp_port, cfg) >+ ? ofproto->ofproto_class->port_set_config(ofport, cfg) > : EOPNOTSUPP); > if (error) { >- VLOG_WARN("%s: dtatapath configuration on port %"PRIu16 >+ VLOG_WARN("%s: datapath configuration on port %"PRIu16 > " (%s) failed (%s)", > ofproto->name, ofp_port, netdev_get_name(ofport->netdev), > ovs_strerror(error)); > >---------------------------------------------------------------------- > > >>> +{ >>> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); >>> + struct ofport_dpif *ofport = ofp_port_to_ofport(ofproto, ofp_port); >>> + >>> + if (!ofport || sset_contains(&ofproto->ghost_ports, >>> + netdev_get_name(ofport->up.netdev))) { >>> + return 0; >>> + } >>> + >>> + return dpif_port_set_config(ofproto->backer->dpif, ofport->odp_port, cfg); >>> +} >>> + >>> +static int >>> port_get_stats(const struct ofport *ofport_, struct netdev_stats *stats) >>> { >>> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); >>> @@ -5609,6 +5624,7 @@ const struct ofproto_class ofproto_dpif_class = { >>> port_query_by_name, >>> port_add, >>> port_del, >>> + port_set_config, >>> port_get_stats, >>> port_dump_start, >>> port_dump_next, >>> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >>> index ae6c08d..883641d 100644 >>> --- a/ofproto/ofproto-provider.h >>> +++ b/ofproto/ofproto-provider.h >>> @@ -972,6 +972,11 @@ struct ofproto_class { >>> * convenient. */ >>> int (*port_del)(struct ofproto *ofproto, ofp_port_t ofp_port); >>> >>> + /* Refreshes dtapath configuration of port number 'ofp_port' in 'ofproto'. >> >> s/dtapath/datapath/ > >Oh, copy-pasted typo. Thanks for pointing this. Fixed in fixup above. > >I'll send v4 when the review of patch #3 will be finished. Sure > >Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 3a13326..79f2aa0 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3543,14 +3543,13 @@ port_del(struct ofproto *ofproto_, ofp_port_t ofp_port) } static int -port_set_config(struct ofproto *ofproto_, ofp_port_t ofp_port, - const struct smap *cfg) +port_set_config(const struct ofport *ofport_, const struct smap *cfg) { - struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); - struct ofport_dpif *ofport = ofp_port_to_ofport(ofproto, ofp_port); + struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto); - if (!ofport || sset_contains(&ofproto->ghost_ports, - netdev_get_name(ofport->up.netdev))) { + if (sset_contains(&ofproto->ghost_ports, + netdev_get_name(ofport->up.netdev))) { return 0; } diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 2fc7452..7156814 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -972,10 +972,9 @@ struct ofproto_class { * convenient. */ int (*port_del)(struct ofproto *ofproto, ofp_port_t ofp_port); - /* Refreshes dtapath configuration of port number 'ofp_port' in 'ofproto'. + /* Refreshes datapath configuration of 'port'. * Returns 0 if successful, otherwise a positive errno value. */ - int (*port_set_config)(struct ofproto *ofproto, ofp_port_t ofp_port, - const struct smap *cfg); + int (*port_set_config)(const struct ofport *port, const struct smap *cfg); /* Get port stats */ int (*port_get_stats)(const struct ofport *port, diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index c66c866..6cd2600 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -2079,7 +2079,7 @@ ofproto_port_del(struct ofproto *ofproto, ofp_port_t ofp_port) return error; } -/* Refreshes dtapath configuration of port number 'ofp_port' in 'ofproto'. +/* Refreshes datapath configuration of port number 'ofp_port' in 'ofproto'. * * This function has no effect if 'ofproto' does not have a port 'ofp_port'. */ void @@ -2097,10 +2097,10 @@ ofproto_port_set_config(struct ofproto *ofproto, ofp_port_t ofp_port, } error = (ofproto->ofproto_class->port_set_config - ? ofproto->ofproto_class->port_set_config(ofproto, ofp_port, cfg) + ? ofproto->ofproto_class->port_set_config(ofport, cfg) : EOPNOTSUPP); if (error) { - VLOG_WARN("%s: dtatapath configuration on port %"PRIu16 + VLOG_WARN("%s: datapath configuration on port %"PRIu16 " (%s) failed (%s)", ofproto->name, ofp_port, netdev_get_name(ofport->netdev), ovs_strerror(error));