On 26/07/2016 06:19, "Ilya Maximets" <i.maxim...@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.maxim...@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.maxim...@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 <diproiet...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
