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" <[email protected]> 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 <[email protected]>
>> ---
>> 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.
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.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
