subject:"\[ovs\-dev\] \[PATCH v3 1\/3\] bridge\: Pass interface's configuration to datapath."

Re: [ovs-dev] [PATCH v3 1/3] bridge: Pass interface's configuration to datapath.

2016-07-26 Thread Daniele Di Proietto






On 26/07/2016 06:19, "Ilya Maximets"  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"  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 
>>> ---
>>> 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 

>
>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(>ghost_ports,
>- netdev_get_name(ofport->up.netdev))) {
>+if (sset_contains(>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),
>

Re: [ovs-dev] [PATCH v3 1/3] bridge: Pass interface's configuration to datapath.

2016-07-26 Thread Ilya Maximets

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"  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 
>> ---
>> 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(>ghost_ports,
- netdev_get_name(ofport->up.netdev))) {
+if (sset_contains(>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(>ghost_ports,
>> +

Re: [ovs-dev] [PATCH v3 1/3] bridge: Pass interface's configuration to datapath.

2016-07-25 Thread Daniele Di Proietto

Thanks for the patch

It looks good to me, a few minor comments inline


On 15/07/2016 04:54, "Ilya Maximets"  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 
>---
> 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/lib/dpif-netdev.c b/lib/dpif-netdev.c
>index 6345944..4643cce 100644
>--- a/lib/dpif-netdev.c
>+++ b/lib/dpif-netdev.c
>@@ -4295,6 +4295,7 @@ const struct dpif_class dpif_netdev_class = {
> dpif_netdev_get_stats,
> dpif_netdev_port_add,
> dpif_netdev_port_del,
>+NULL,   /* port_set_config */
> dpif_netdev_port_query_by_number,
> dpif_netdev_port_query_by_name,
> NULL,   /* port_get_pid */
>diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>index e2bea23..2f939ae 100644
>--- a/lib/dpif-netlink.c
>+++ b/lib/dpif-netlink.c
>@@ -2348,6 +2348,7 @@ const struct dpif_class dpif_netlink_class = {
> dpif_netlink_get_stats,
> dpif_netlink_port_add,
> dpif_netlink_port_del,
>+NULL,   /* port_set_config */
> dpif_netlink_port_query_by_number,
> dpif_netlink_port_query_by_name,
> dpif_netlink_port_get_pid,
>diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>index 25f4280..21fb0ba 100644
>--- a/lib/dpif-provider.h
>+++ b/lib/dpif-provider.h
>@@ -167,6 +167,11 @@ struct dpif_class {
> /* Removes port numbered 'port_no' from 'dpif'. */
> int (*port_del)(struct dpif *dpif, odp_port_t port_no);
> 
>+/* Refreshes configuration of 'dpif's port. The implementation might
>+ * postpone applying the changes until run() is called. */
>+int (*port_set_config)(struct dpif *dpif, odp_port_t port_no,
>+   const struct smap *cfg);
>+
> /* Queries 'dpif' for a port with the given 'port_no' or 'devname'.
>  * If 'port' is not null, stores information about the port into
>  * '*port' if successful.
>diff --git a/lib/dpif.c b/lib/dpif.c
>index 5f1be41..f6e5338 100644
>--- a/lib/dpif.c
>+++ b/lib/dpif.c
>@@ -610,6 +610,23 @@ dpif_port_exists(const struct dpif *dpif, const char 
>*devname)
> return !error;
> }
> 
>+/* Refreshes configuration of 'dpif's port. */
>+int
>+dpif_port_set_config(struct dpif *dpif, odp_port_t port_no,
>+ const struct smap *cfg)
>+{
>+int error = 0;
>+
>+if (dpif->dpif_class->port_set_config) {
>+error = dpif->dpif_class->port_set_config(dpif, port_no, cfg);
>+if (error) {
>+log_operation(dpif, "port_set_config", error);
>+}
>+}
>+
>+return error;
>+}
>+
> /* Looks up port number 'port_no' in 'dpif'.  On success, returns 0 and
>  * initializes '*port' appropriately; on failure, returns a positive errno
>  * value.
>diff --git a/lib/dpif.h b/lib/dpif.h
>index 981868c..a7c5097 100644
>--- a/lib/dpif.h
>+++ b/lib/dpif.h
>@@ -839,6 +839,7 @@ void dpif_register_upcall_cb(struct dpif *, 
>upcall_callback *, void *aux);
> int dpif_recv_set(struct dpif *, bool enable);
> int dpif_handlers_set(struct dpif *, uint32_t n_handlers);
> int dpif_poll_threads_set(struct dpif *, const char *cmask);
>+int dpif_port_set_config(struct dpif *, odp_port_t, const struct smap *cfg);
> int dpif_recv(struct dpif *, uint32_t handler_id, struct dpif_upcall *,
>   struct ofpbuf *);
> void dpif_recv_purge(struct dpif *);
>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?

>+{
>+struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>+struct ofport_dpif *ofport = ofp_port_to_ofport(ofproto, ofp_port);
>+
>+if (!ofport || sset_contains(>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_,

[ovs-dev] [PATCH v3 1/3] bridge: Pass interface's configuration to datapath.

2016-07-15 Thread Ilya Maximets

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 
---
 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/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 6345944..4643cce 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4295,6 +4295,7 @@ const struct dpif_class dpif_netdev_class = {
 dpif_netdev_get_stats,
 dpif_netdev_port_add,
 dpif_netdev_port_del,
+NULL,   /* port_set_config */
 dpif_netdev_port_query_by_number,
 dpif_netdev_port_query_by_name,
 NULL,   /* port_get_pid */
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index e2bea23..2f939ae 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2348,6 +2348,7 @@ const struct dpif_class dpif_netlink_class = {
 dpif_netlink_get_stats,
 dpif_netlink_port_add,
 dpif_netlink_port_del,
+NULL,   /* port_set_config */
 dpif_netlink_port_query_by_number,
 dpif_netlink_port_query_by_name,
 dpif_netlink_port_get_pid,
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 25f4280..21fb0ba 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -167,6 +167,11 @@ struct dpif_class {
 /* Removes port numbered 'port_no' from 'dpif'. */
 int (*port_del)(struct dpif *dpif, odp_port_t port_no);
 
+/* Refreshes configuration of 'dpif's port. The implementation might
+ * postpone applying the changes until run() is called. */
+int (*port_set_config)(struct dpif *dpif, odp_port_t port_no,
+   const struct smap *cfg);
+
 /* Queries 'dpif' for a port with the given 'port_no' or 'devname'.
  * If 'port' is not null, stores information about the port into
  * '*port' if successful.
diff --git a/lib/dpif.c b/lib/dpif.c
index 5f1be41..f6e5338 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -610,6 +610,23 @@ dpif_port_exists(const struct dpif *dpif, const char 
*devname)
 return !error;
 }
 
+/* Refreshes configuration of 'dpif's port. */
+int
+dpif_port_set_config(struct dpif *dpif, odp_port_t port_no,
+ const struct smap *cfg)
+{
+int error = 0;
+
+if (dpif->dpif_class->port_set_config) {
+error = dpif->dpif_class->port_set_config(dpif, port_no, cfg);
+if (error) {
+log_operation(dpif, "port_set_config", error);
+}
+}
+
+return error;
+}
+
 /* Looks up port number 'port_no' in 'dpif'.  On success, returns 0 and
  * initializes '*port' appropriately; on failure, returns a positive errno
  * value.
diff --git a/lib/dpif.h b/lib/dpif.h
index 981868c..a7c5097 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -839,6 +839,7 @@ void dpif_register_upcall_cb(struct dpif *, upcall_callback 
*, void *aux);
 int dpif_recv_set(struct dpif *, bool enable);
 int dpif_handlers_set(struct dpif *, uint32_t n_handlers);
 int dpif_poll_threads_set(struct dpif *, const char *cmask);
+int dpif_port_set_config(struct dpif *, odp_port_t, const struct smap *cfg);
 int dpif_recv(struct dpif *, uint32_t handler_id, struct dpif_upcall *,
   struct ofpbuf *);
 void dpif_recv_purge(struct dpif *);
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)
+{
+struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
+struct ofport_dpif *ofport = ofp_port_to_ofport(ofproto, ofp_port);
+
+if (!ofport || sset_contains(>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

Re: [ovs-dev] [PATCH v3 1/3] bridge: Pass interface's configuration to datapath.

Re: [ovs-dev] [PATCH v3 1/3] bridge: Pass interface's configuration to datapath.

Re: [ovs-dev] [PATCH v3 1/3] bridge: Pass interface's configuration to datapath.

[ovs-dev] [PATCH v3 1/3] bridge: Pass interface's configuration to datapath.

4 matches

Site Navigation

Mail list logo

Footer information