| Message ID | 1495972813-13475-6-git-send-email-roid@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, May 28, 2017 at 02:59:47PM +0300, Roi Dayan wrote: > From: Paul Blakey <paulb@mellanox.com> > > Add a new API interface for offloading dpif flows to netdev. > The API consist on the following: > flow_put - offload a new flow > flow_get - query an offloaded flow > flow_del - delete an offloaded flow > flow_flush - flush all offloaded flows > flow_dump_* - dump all offloaded flows > > In upcoming commits we will introduce an implementation of this > API for netdev-linux. > > Signed-off-by: Paul Blakey <paulb@mellanox.com> > Reviewed-by: Roi Dayan <roid@mellanox.com> > Reviewed-by: Simon Horman <simon.horman@netronome.com> This looks good to me, modulo the minor nits below, and I would be happy to apply it if someone provided a review. > diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c > new file mode 100644 > index 0000000..46017d8 > --- /dev/null > +++ b/lib/netdev-tc-offloads.c > @@ -0,0 +1,115 @@ ... > +int > +netdev_tc_init_flow_api(struct netdev *netdev OVS_UNUSED) > +{ > + return 0; > +} > + This introduces a blank line at the end of a file. ... > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 39093e8..2cad5cb 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -847,7 +847,16 @@ get_stats(const struct netdev *netdev, struct netdev_stats *stats) ... > +bool > +netdev_flow_dump_next(struct netdev_flow_dump *dump, > + struct match *match, > + struct nlattr **actions, > + struct dpif_flow_stats *stats, > + ovs_u128 *ufid, > + struct ofpbuf *rbuffer, > + struct ofpbuf *wbuffer) > +{ It looks some of the lines in the function declaration could be consolidated. > + const struct netdev_class *class = dump->netdev->netdev_class; > + > + return (class->flow_dump_next > + ? class->flow_dump_next(dump, match, actions, stats, ufid, > + rbuffer, wbuffer) > + : false); > +} > + > +int > +netdev_flow_put(struct netdev *netdev, struct match *match, > + struct nlattr *actions, size_t act_len, > + const ovs_u128 *ufid, struct offload_info *info, > + struct dpif_flow_stats *stats) > +{ Like these ones are. ...
On 28 May 2017 at 04:59, Roi Dayan <roid@mellanox.com> wrote: > From: Paul Blakey <paulb@mellanox.com> > > Add a new API interface for offloading dpif flows to netdev. > The API consist on the following: > flow_put - offload a new flow > flow_get - query an offloaded flow > flow_del - delete an offloaded flow > flow_flush - flush all offloaded flows > flow_dump_* - dump all offloaded flows > > In upcoming commits we will introduce an implementation of this > API for netdev-linux. > > Signed-off-by: Paul Blakey <paulb@mellanox.com> > Reviewed-by: Roi Dayan <roid@mellanox.com> > Reviewed-by: Simon Horman <simon.horman@netronome.com> > --- <snip> > @@ -769,6 +777,67 @@ struct netdev_class { > > /* Discards all packets waiting to be received from 'rx'. */ > int (*rxq_drain)(struct netdev_rxq *rx); > + > + /* ## -------------------------------- ## */ > + /* ## netdev flow offloading functions ## */ > + /* ## -------------------------------- ## */ > + > + /* If a particular netdev class does not support offloading flows, > + * all these function pointers must be NULL. */ > + > + /* Flush all offloaded flows from a netdev. > + * Return 0 if successful, otherwise returns a positive errno value. */ > + int (*flow_flush)(struct netdev *); > + > + /* Flow dumping interface. > + * > + * This is the back-end for the flow dumping interface described in > + * dpif.h. Please read the comments there first, because this code > + * closely follows it. > + * > + * 'flow_dump_create' is being executed in a dpif thread so there is > + * no need for 'flow_dump_thread_create' implementation. I find this comment a bit confusing, but it's a good thing it was here because it raises a couple of discussion points. 'flow_dump_thread_create', perhaps poorly named, doesn't create a thread, but allocates memory for per-thread state so that each thread may dump safely in parallel while operating on an independent netlink dump and independent buffers. I guess that in the DPIF flow dump there is global dump state and per-thread state, while in this netdev flow dump API there is only global state? Describing that this interface doesn't need something that isn't being defined is a bit strange. If it's not needed, then we probably don't need to describe why it's not needed here since there's no such function. Then, the comment can be dropped. > + * On success returns allocated netdev_flow_dump data, on failure returns ^ returns allocated netdev_flow_dump_data "and returns 0"...? > + * positive errno. */ > + int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump); > + int (*flow_dump_destroy)(struct netdev_flow_dump *); > + > + /* Returns true while there are more flows to dump. s/while/if/ > + * rbuffer is used as a temporary buffer and needs to be pre allocated > + * by the caller. while there are more flows the same rbuffer should > + * be provided. wbuffer is used to store dumped actions and needs to be > + * pre allocated by the caller. */ I have a couple of extra questions which this description could be expanded to answer: Who is responsible for freeing 'rbuffer' and 'wbuffer'? I expect the caller, but this could be more explicit. Are the pointers that are returned valid beyond the next call to flow_dump_next()? Please also capitalize the starts of sentences. (I'll say this once, but it applies to several of the comments). > + bool (*flow_dump_next)(struct netdev_flow_dump *, struct match *, > + struct nlattr **actions, > + struct dpif_flow_stats *stats, ovs_u128 *ufid, > + struct ofpbuf *rbuffer, struct ofpbuf *wbuffer); > + > + /* Offload the given flow on netdev. > + * To modify a flow, use the same ufid. > + * actions are in netlink format, as with struct dpif_flow_put. Is this "OVS netlink format" or "TC flower netlink format"? > + * info is extra info needed to offload the flow. > + * Read the comments on 'struct dpif_flow_put' in dpif.h about stats. This sentence about stats is more descriptive if it states something such as: 'stats' is populated according to the rules set out in the description above 'struct dpif_flow_del'. > + * Return 0 if successful, otherwise returns a positive errno value. */ > + int (*flow_put)(struct netdev *, struct match *, struct nlattr *actions, > + size_t actions_len, const ovs_u128 *ufid, > + struct offload_info *info, struct dpif_flow_stats *); > + > + /* Queries a flow specified by ufid on netdev. > + * Fills output buffer as wbuffer in flow_dump_next. > + * the buffer needs to be pre allocated. > + * Return 0 if successful, otherwise returns a positive errno value. */ How is the caller expected to use the parameters? If it is expected to use the buffer and interpret its data, that should be described. If not, then 'buffer' should be described as temporary storage for fetching the requested flow, and the other parameters should be described in more detail. The state of each pointer should also be described depending on the success or failure of the function. Put another way, what are the input and output parameters? Looking around, there's still several parameters left undefined in the APIs throughout this patch. Please also look at the others. > + int (*flow_get)(struct netdev *, struct match *, struct nlattr **actions, > + const ovs_u128 *ufid, struct dpif_flow_stats *, > + struct ofpbuf *wbuffer); > + > + /* Delete a flow specified by ufid from netdev. > + * Read the comments on 'struct dpif_flow_del' in dpif.h about stats. Same stats comment as flow_put(). > + * Return 0 if successful, otherwise returns a positive errno value. */ > + int (*flow_del)(struct netdev *, const ovs_u128 *ufid, > + struct dpif_flow_stats *); > + > + /* Initializies the netdev flow api. */ It seems that you missed the description of the return code in the comment above. > + int (*init_flow_api)(struct netdev *); > }; > > int netdev_register_provider(const struct netdev_class *); <snip> > diff --git a/lib/netdev.h b/lib/netdev.h > index d6c07c1..17890e6 100644 > --- a/lib/netdev.h > +++ b/lib/netdev.h > @@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct dp_packet_batch *, > bool may_steal, bool concurrent_txq); > void netdev_send_wait(struct netdev *, int qid); > > +/* Flow offloading. */ > +struct offload_info { > + const void *port_hmap_obj; /* To query ports info from netdev port map */ > + ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */ > +}; I see that the port_hmap still isn't defined as an actual type. If you don't mind refreshing my memory, was there a hurdle in defining this more formally? I plan on stopping here with my review for now.
On 30/05/2017 11:09, Simon Horman wrote: > On Sun, May 28, 2017 at 02:59:47PM +0300, Roi Dayan wrote: >> From: Paul Blakey <paulb@mellanox.com> >> >> Add a new API interface for offloading dpif flows to netdev. >> The API consist on the following: >> flow_put - offload a new flow >> flow_get - query an offloaded flow >> flow_del - delete an offloaded flow >> flow_flush - flush all offloaded flows >> flow_dump_* - dump all offloaded flows >> >> In upcoming commits we will introduce an implementation of this >> API for netdev-linux. >> >> Signed-off-by: Paul Blakey <paulb@mellanox.com> >> Reviewed-by: Roi Dayan <roid@mellanox.com> >> Reviewed-by: Simon Horman <simon.horman@netronome.com> > > This looks good to me, modulo the minor nits below, and I would > be happy to apply it if someone provided a review. > > >> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c >> new file mode 100644 >> index 0000000..46017d8 >> --- /dev/null >> +++ b/lib/netdev-tc-offloads.c >> @@ -0,0 +1,115 @@ > > ... > >> +int >> +netdev_tc_init_flow_api(struct netdev *netdev OVS_UNUSED) >> +{ >> + return 0; >> +} >> + > > This introduces a blank line at the end of a file. > ok > ... > >> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c >> index 39093e8..2cad5cb 100644 >> --- a/lib/netdev-vport.c >> +++ b/lib/netdev-vport.c >> @@ -847,7 +847,16 @@ get_stats(const struct netdev *netdev, struct netdev_stats *stats) > > ... > >> +bool >> +netdev_flow_dump_next(struct netdev_flow_dump *dump, >> + struct match *match, >> + struct nlattr **actions, >> + struct dpif_flow_stats *stats, >> + ovs_u128 *ufid, >> + struct ofpbuf *rbuffer, >> + struct ofpbuf *wbuffer) >> +{ > > It looks some of the lines in the function declaration could be > consolidated. ok > >> + const struct netdev_class *class = dump->netdev->netdev_class; >> + >> + return (class->flow_dump_next >> + ? class->flow_dump_next(dump, match, actions, stats, ufid, >> + rbuffer, wbuffer) >> + : false); >> +} >> + >> +int >> +netdev_flow_put(struct netdev *netdev, struct match *match, >> + struct nlattr *actions, size_t act_len, >> + const ovs_u128 *ufid, struct offload_info *info, >> + struct dpif_flow_stats *stats) >> +{ > > Like these ones are. > > ... >
On 31/05/2017 04:37, Joe Stringer wrote: > On 28 May 2017 at 04:59, Roi Dayan <roid@mellanox.com> wrote: >> From: Paul Blakey <paulb@mellanox.com> >> >> Add a new API interface for offloading dpif flows to netdev. >> The API consist on the following: >> flow_put - offload a new flow >> flow_get - query an offloaded flow >> flow_del - delete an offloaded flow >> flow_flush - flush all offloaded flows >> flow_dump_* - dump all offloaded flows >> >> In upcoming commits we will introduce an implementation of this >> API for netdev-linux. >> >> Signed-off-by: Paul Blakey <paulb@mellanox.com> >> Reviewed-by: Roi Dayan <roid@mellanox.com> >> Reviewed-by: Simon Horman <simon.horman@netronome.com> >> --- > > <snip> > >> @@ -769,6 +777,67 @@ struct netdev_class { >> >> /* Discards all packets waiting to be received from 'rx'. */ >> int (*rxq_drain)(struct netdev_rxq *rx); >> + >> + /* ## -------------------------------- ## */ >> + /* ## netdev flow offloading functions ## */ >> + /* ## -------------------------------- ## */ >> + >> + /* If a particular netdev class does not support offloading flows, >> + * all these function pointers must be NULL. */ >> + >> + /* Flush all offloaded flows from a netdev. >> + * Return 0 if successful, otherwise returns a positive errno value. */ >> + int (*flow_flush)(struct netdev *); >> + >> + /* Flow dumping interface. >> + * >> + * This is the back-end for the flow dumping interface described in >> + * dpif.h. Please read the comments there first, because this code >> + * closely follows it. >> + * >> + * 'flow_dump_create' is being executed in a dpif thread so there is >> + * no need for 'flow_dump_thread_create' implementation. > > I find this comment a bit confusing, but it's a good thing it was here > because it raises a couple of discussion points. > > 'flow_dump_thread_create', perhaps poorly named, doesn't create a > thread, but allocates memory for per-thread state so that each thread > may dump safely in parallel while operating on an independent netlink > dump and independent buffers. I guess that in the DPIF flow dump there > is global dump state and per-thread state, while in this netdev flow > dump API there is only global state? > > Describing that this interface doesn't need something that isn't being > defined is a bit strange. If it's not needed, then we probably don't > need to describe why it's not needed here since there's no such > function. Then, the comment can be dropped. > >> + * On success returns allocated netdev_flow_dump data, on failure returns > > ^ returns allocated netdev_flow_dump_data "and returns 0"...? > >> + * positive errno. */ >> + int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump); >> + int (*flow_dump_destroy)(struct netdev_flow_dump *); >> + >> + /* Returns true while there are more flows to dump. > > s/while/if/ > >> + * rbuffer is used as a temporary buffer and needs to be pre allocated >> + * by the caller. while there are more flows the same rbuffer should >> + * be provided. wbuffer is used to store dumped actions and needs to be >> + * pre allocated by the caller. */ > > I have a couple of extra questions which this description could be > expanded to answer: > > Who is responsible for freeing 'rbuffer' and 'wbuffer'? I expect the > caller, but this could be more explicit. caller. as noted the function expects them to be pre allocated. > > Are the pointers that are returned valid beyond the next call to > flow_dump_next()? yes. what can we add to make it clear? > > Please also capitalize the starts of sentences. (I'll say this once, > but it applies to several of the comments). ok > >> + bool (*flow_dump_next)(struct netdev_flow_dump *, struct match *, >> + struct nlattr **actions, >> + struct dpif_flow_stats *stats, ovs_u128 *ufid, >> + struct ofpbuf *rbuffer, struct ofpbuf *wbuffer); >> + >> + /* Offload the given flow on netdev. >> + * To modify a flow, use the same ufid. >> + * actions are in netlink format, as with struct dpif_flow_put. > > Is this "OVS netlink format" or "TC flower netlink format"? netlink > >> + * info is extra info needed to offload the flow. >> + * Read the comments on 'struct dpif_flow_put' in dpif.h about stats. > > This sentence about stats is more descriptive if it states something such as: > > 'stats' is populated according to the rules set out in the description > above 'struct dpif_flow_del'. > ok >> + * Return 0 if successful, otherwise returns a positive errno value. */ >> + int (*flow_put)(struct netdev *, struct match *, struct nlattr *actions, >> + size_t actions_len, const ovs_u128 *ufid, >> + struct offload_info *info, struct dpif_flow_stats *); >> + >> + /* Queries a flow specified by ufid on netdev. >> + * Fills output buffer as wbuffer in flow_dump_next. >> + * the buffer needs to be pre allocated. >> + * Return 0 if successful, otherwise returns a positive errno value. */ > > How is the caller expected to use the parameters? If it is expected to > use the buffer and interpret its data, that should be described. If > not, then 'buffer' should be described as temporary storage for > fetching the requested flow, and the other parameters should be > described in more detail. The state of each pointer should also be > described depending on the success or failure of the function. ok > > Put another way, what are the input and output parameters? output params are stats and wbuffer. > > Looking around, there's still several parameters left undefined in the > APIs throughout this patch. Please also look at the others. > ok >> + int (*flow_get)(struct netdev *, struct match *, struct nlattr **actions, >> + const ovs_u128 *ufid, struct dpif_flow_stats *, >> + struct ofpbuf *wbuffer); >> + >> + /* Delete a flow specified by ufid from netdev. >> + * Read the comments on 'struct dpif_flow_del' in dpif.h about stats. > > Same stats comment as flow_put(). > >> + * Return 0 if successful, otherwise returns a positive errno value. */ >> + int (*flow_del)(struct netdev *, const ovs_u128 *ufid, >> + struct dpif_flow_stats *); >> + >> + /* Initializies the netdev flow api. */ > > It seems that you missed the description of the return code in the > comment above. ok > >> + int (*init_flow_api)(struct netdev *); >> }; >> >> int netdev_register_provider(const struct netdev_class *); > > <snip> > >> diff --git a/lib/netdev.h b/lib/netdev.h >> index d6c07c1..17890e6 100644 >> --- a/lib/netdev.h >> +++ b/lib/netdev.h >> @@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct dp_packet_batch *, >> bool may_steal, bool concurrent_txq); >> void netdev_send_wait(struct netdev *, int qid); >> >> +/* Flow offloading. */ >> +struct offload_info { >> + const void *port_hmap_obj; /* To query ports info from netdev port map */ >> + ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */ >> +}; > > I see that the port_hmap still isn't defined as an actual type. If you > don't mind refreshing my memory, was there a hurdle in defining this > more formally? > > I plan on stopping here with my review for now. > Hi Joe, I accidentally skipped your comments here for V10. I'll address them in the next update. We skipped addressing port_hmap_obj as we also wanted to move it from global to be per dpif which I think got me stuck somewhere in the process. I don't remember the reason. maybe we can still do as a first step changing this void* to some type but still be global and later to be per dpif. in any case can we address this in a later commit ? Thanks, Roi
On 8 June 2017 at 05:05, Roi Dayan <roid@mellanox.com> wrote: > > > On 31/05/2017 04:37, Joe Stringer wrote: >> >> On 28 May 2017 at 04:59, Roi Dayan <roid@mellanox.com> wrote: >>> >>> From: Paul Blakey <paulb@mellanox.com> >>> >>> Add a new API interface for offloading dpif flows to netdev. >>> The API consist on the following: >>> flow_put - offload a new flow >>> flow_get - query an offloaded flow >>> flow_del - delete an offloaded flow >>> flow_flush - flush all offloaded flows >>> flow_dump_* - dump all offloaded flows >>> >>> In upcoming commits we will introduce an implementation of this >>> API for netdev-linux. >>> >>> Signed-off-by: Paul Blakey <paulb@mellanox.com> >>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>> Reviewed-by: Simon Horman <simon.horman@netronome.com> >>> --- >> >> >> <snip> >> >>> @@ -769,6 +777,67 @@ struct netdev_class { >>> >>> /* Discards all packets waiting to be received from 'rx'. */ >>> int (*rxq_drain)(struct netdev_rxq *rx); >>> + >>> + /* ## -------------------------------- ## */ >>> + /* ## netdev flow offloading functions ## */ >>> + /* ## -------------------------------- ## */ >>> + >>> + /* If a particular netdev class does not support offloading flows, >>> + * all these function pointers must be NULL. */ >>> + >>> + /* Flush all offloaded flows from a netdev. >>> + * Return 0 if successful, otherwise returns a positive errno value. >>> */ >>> + int (*flow_flush)(struct netdev *); >>> + >>> + /* Flow dumping interface. >>> + * >>> + * This is the back-end for the flow dumping interface described in >>> + * dpif.h. Please read the comments there first, because this code >>> + * closely follows it. >>> + * >>> + * 'flow_dump_create' is being executed in a dpif thread so there is >>> + * no need for 'flow_dump_thread_create' implementation. >> >> >> I find this comment a bit confusing, but it's a good thing it was here >> because it raises a couple of discussion points. >> >> 'flow_dump_thread_create', perhaps poorly named, doesn't create a >> thread, but allocates memory for per-thread state so that each thread >> may dump safely in parallel while operating on an independent netlink >> dump and independent buffers. I guess that in the DPIF flow dump there >> is global dump state and per-thread state, while in this netdev flow >> dump API there is only global state? >> >> Describing that this interface doesn't need something that isn't being >> defined is a bit strange. If it's not needed, then we probably don't >> need to describe why it's not needed here since there's no such >> function. Then, the comment can be dropped. >> >>> + * On success returns allocated netdev_flow_dump data, on failure >>> returns >> >> >> ^ returns allocated netdev_flow_dump_data "and returns 0"...? >> >>> + * positive errno. */ >>> + int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump >>> **dump); >>> + int (*flow_dump_destroy)(struct netdev_flow_dump *); >>> + >>> + /* Returns true while there are more flows to dump. >> >> >> s/while/if/ >> >>> + * rbuffer is used as a temporary buffer and needs to be pre >>> allocated >>> + * by the caller. while there are more flows the same rbuffer should >>> + * be provided. wbuffer is used to store dumped actions and needs to >>> be >>> + * pre allocated by the caller. */ >> >> >> I have a couple of extra questions which this description could be >> expanded to answer: >> >> Who is responsible for freeing 'rbuffer' and 'wbuffer'? I expect the >> caller, but this could be more explicit. > > > caller. as noted the function expects them to be pre allocated. Makes sense, but to be precise in the API documentation it should probably state that the caller is responsible for freeing those buffers. >> Are the pointers that are returned valid beyond the next call to >> flow_dump_next()? > > > yes. what can we add to make it clear? Hmm, ok. Usually when you make a call to the DPIF layer flow_dump_next, you provide a buffer which gets populated and the flows point within the buffer (round 1). Once you call flow_dump_next again after that (round 2), then the flows in round 1 are not guaranteed to be valid, because they point within the buffer that is getting manipulated by this function. The DPIF layer describes this limitation in its documentation, which implies that callers who wish to preserve the flow beyond the next (round 2) call to dump_next would have to make a copy. Is that the case here too? I guess that if there is not such a restriction, then I'm not sure if there's anything to describe. > Hi Joe, > > I accidentally skipped your comments here for V10. > I'll address them in the next update. OK, thanks. > We skipped addressing port_hmap_obj as we also wanted to move it from > global to be per dpif which I think got me stuck somewhere in the > process. I don't remember the reason. > maybe we can still do as a first step changing this void* to some > type but still be global and later to be per dpif. > in any case can we address this in a later commit ? Sure, that sounds like a reasonable approach.
On 08/06/2017 20:08, Joe Stringer wrote: > On 8 June 2017 at 05:05, Roi Dayan <roid@mellanox.com> wrote: >> >> >> On 31/05/2017 04:37, Joe Stringer wrote: >>> >>> On 28 May 2017 at 04:59, Roi Dayan <roid@mellanox.com> wrote: >>>> >>>> From: Paul Blakey <paulb@mellanox.com> >>>> >>>> Add a new API interface for offloading dpif flows to netdev. >>>> The API consist on the following: >>>> flow_put - offload a new flow >>>> flow_get - query an offloaded flow >>>> flow_del - delete an offloaded flow >>>> flow_flush - flush all offloaded flows >>>> flow_dump_* - dump all offloaded flows >>>> >>>> In upcoming commits we will introduce an implementation of this >>>> API for netdev-linux. >>>> >>>> Signed-off-by: Paul Blakey <paulb@mellanox.com> >>>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>>> Reviewed-by: Simon Horman <simon.horman@netronome.com> >>>> --- >>> >>> >>> <snip> >>> >>>> @@ -769,6 +777,67 @@ struct netdev_class { >>>> >>>> /* Discards all packets waiting to be received from 'rx'. */ >>>> int (*rxq_drain)(struct netdev_rxq *rx); >>>> + >>>> + /* ## -------------------------------- ## */ >>>> + /* ## netdev flow offloading functions ## */ >>>> + /* ## -------------------------------- ## */ >>>> + >>>> + /* If a particular netdev class does not support offloading flows, >>>> + * all these function pointers must be NULL. */ >>>> + >>>> + /* Flush all offloaded flows from a netdev. >>>> + * Return 0 if successful, otherwise returns a positive errno value. >>>> */ >>>> + int (*flow_flush)(struct netdev *); >>>> + >>>> + /* Flow dumping interface. >>>> + * >>>> + * This is the back-end for the flow dumping interface described in >>>> + * dpif.h. Please read the comments there first, because this code >>>> + * closely follows it. >>>> + * >>>> + * 'flow_dump_create' is being executed in a dpif thread so there is >>>> + * no need for 'flow_dump_thread_create' implementation. >>> >>> >>> I find this comment a bit confusing, but it's a good thing it was here >>> because it raises a couple of discussion points. >>> >>> 'flow_dump_thread_create', perhaps poorly named, doesn't create a >>> thread, but allocates memory for per-thread state so that each thread >>> may dump safely in parallel while operating on an independent netlink >>> dump and independent buffers. I guess that in the DPIF flow dump there >>> is global dump state and per-thread state, while in this netdev flow >>> dump API there is only global state? >>> >>> Describing that this interface doesn't need something that isn't being >>> defined is a bit strange. If it's not needed, then we probably don't >>> need to describe why it's not needed here since there's no such >>> function. Then, the comment can be dropped. >>> >>>> + * On success returns allocated netdev_flow_dump data, on failure >>>> returns >>> >>> >>> ^ returns allocated netdev_flow_dump_data "and returns 0"...? >>> >>>> + * positive errno. */ >>>> + int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump >>>> **dump); >>>> + int (*flow_dump_destroy)(struct netdev_flow_dump *); >>>> + >>>> + /* Returns true while there are more flows to dump. >>> >>> >>> s/while/if/ >>> >>>> + * rbuffer is used as a temporary buffer and needs to be pre >>>> allocated >>>> + * by the caller. while there are more flows the same rbuffer should >>>> + * be provided. wbuffer is used to store dumped actions and needs to >>>> be >>>> + * pre allocated by the caller. */ >>> >>> >>> I have a couple of extra questions which this description could be >>> expanded to answer: >>> >>> Who is responsible for freeing 'rbuffer' and 'wbuffer'? I expect the >>> caller, but this could be more explicit. >> >> >> caller. as noted the function expects them to be pre allocated. > > Makes sense, but to be precise in the API documentation it should > probably state that the caller is responsible for freeing those > buffers. > >>> Are the pointers that are returned valid beyond the next call to >>> flow_dump_next()? >> >> >> yes. what can we add to make it clear? > > Hmm, ok. Usually when you make a call to the DPIF layer > flow_dump_next, you provide a buffer which gets populated and the > flows point within the buffer (round 1). Once you call flow_dump_next > again after that (round 2), then the flows in round 1 are not > guaranteed to be valid, because they point within the buffer that is > getting manipulated by this function. The DPIF layer describes this > limitation in its documentation, which implies that callers who wish > to preserve the flow beyond the next (round 2) call to dump_next would > have to make a copy. Is that the case here too? I guess that if there > is not such a restriction, then I'm not sure if there's anything to > describe. right. it's the same limitation as wbuffer is the output buffer. so it depends if the caller pass same address or a different address. > >> Hi Joe, >> >> I accidentally skipped your comments here for V10. >> I'll address them in the next update. > > OK, thanks. > >> We skipped addressing port_hmap_obj as we also wanted to move it from >> global to be per dpif which I think got me stuck somewhere in the >> process. I don't remember the reason. >> maybe we can still do as a first step changing this void* to some >> type but still be global and later to be per dpif. >> in any case can we address this in a later commit ? > > Sure, that sounds like a reasonable approach. >
diff --git a/lib/automake.mk b/lib/automake.mk index 3d57610..a7c8009 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -354,6 +354,8 @@ lib_libopenvswitch_la_SOURCES += \ lib/dpif-netlink.h \ lib/tc.h \ lib/tc.c \ + lib/netdev-tc-offloads.h \ + lib/netdev-tc-offloads.c \ lib/if-notifier.c \ lib/if-notifier.h \ lib/netdev-linux.c \ diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c index 94c515d..5b54d79 100644 --- a/lib/netdev-bsd.c +++ b/lib/netdev-bsd.c @@ -1547,6 +1547,8 @@ netdev_bsd_update_flags(struct netdev *netdev_, enum netdev_flags off, netdev_bsd_rxq_recv, \ netdev_bsd_rxq_wait, \ netdev_bsd_rxq_drain, \ + \ + NO_OFFLOAD_API \ } const struct netdev_class netdev_bsd_class = diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ddc651b..9ad96cd 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -3314,6 +3314,7 @@ unlock: RXQ_RECV, \ NULL, /* rx_wait */ \ NULL, /* rxq_drain */ \ + NO_OFFLOAD_API \ } static const struct netdev_class dpdk_class = diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 0657434..7e1383f 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -1409,6 +1409,8 @@ netdev_dummy_update_flags(struct netdev *netdev_, netdev_dummy_rxq_recv, \ netdev_dummy_rxq_wait, \ netdev_dummy_rxq_drain, \ + \ + NO_OFFLOAD_API \ } static const struct netdev_class dummy_class = diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 7a0517b..65042c6 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -73,6 +73,7 @@ #include "openvswitch/vlog.h" #include "util.h" #include "tc.h" +#include "netdev-tc-offloads.h" VLOG_DEFINE_THIS_MODULE(netdev_linux); @@ -2783,7 +2784,8 @@ netdev_linux_update_flags(struct netdev *netdev_, enum netdev_flags off, } #define NETDEV_LINUX_CLASS(NAME, CONSTRUCT, GET_STATS, \ - GET_FEATURES, GET_STATUS) \ + GET_FEATURES, GET_STATUS, \ + FLOW_OFFLOAD_API) \ { \ NAME, \ false, /* is_pmd */ \ @@ -2852,6 +2854,8 @@ netdev_linux_update_flags(struct netdev *netdev_, enum netdev_flags off, netdev_linux_rxq_recv, \ netdev_linux_rxq_wait, \ netdev_linux_rxq_drain, \ + \ + FLOW_OFFLOAD_API \ } const struct netdev_class netdev_linux_class = @@ -2860,7 +2864,8 @@ const struct netdev_class netdev_linux_class = netdev_linux_construct, netdev_linux_get_stats, netdev_linux_get_features, - netdev_linux_get_status); + netdev_linux_get_status, + LINUX_FLOW_OFFLOAD_API); const struct netdev_class netdev_tap_class = NETDEV_LINUX_CLASS( @@ -2868,7 +2873,8 @@ const struct netdev_class netdev_tap_class = netdev_linux_construct_tap, netdev_tap_get_stats, netdev_linux_get_features, - netdev_linux_get_status); + netdev_linux_get_status, + NO_OFFLOAD_API); const struct netdev_class netdev_internal_class = NETDEV_LINUX_CLASS( @@ -2876,7 +2882,8 @@ const struct netdev_class netdev_internal_class = netdev_linux_construct, netdev_internal_get_stats, NULL, /* get_features */ - netdev_internal_get_status); + netdev_internal_get_status, + NO_OFFLOAD_API); #define CODEL_N_QUEUES 0x0000 diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h index 0c61bc9..d944691 100644 --- a/lib/netdev-linux.h +++ b/lib/netdev-linux.h @@ -28,4 +28,13 @@ struct netdev; int netdev_linux_ethtool_set_flag(struct netdev *netdev, uint32_t flag, const char *flag_name, bool enable); +#define LINUX_FLOW_OFFLOAD_API \ + netdev_tc_flow_flush, \ + netdev_tc_flow_dump_create, \ + netdev_tc_flow_dump_destroy, \ + netdev_tc_flow_dump_next, \ + netdev_tc_flow_put, \ + netdev_tc_flow_get, \ + netdev_tc_flow_del, \ + netdev_tc_init_flow_api #endif /* netdev-linux.h */ diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h index 8346fc4..e8bfdc8 100644 --- a/lib/netdev-provider.h +++ b/lib/netdev-provider.h @@ -115,6 +115,14 @@ struct netdev_rxq { struct netdev *netdev_rxq_get_netdev(const struct netdev_rxq *); + +struct netdev_flow_dump { + struct netdev *netdev; + odp_port_t port; + struct nl_dump *nl_dump; + bool terse; +}; + /* Network device class structure, to be defined by each implementation of a * network device. * @@ -769,6 +777,67 @@ struct netdev_class { /* Discards all packets waiting to be received from 'rx'. */ int (*rxq_drain)(struct netdev_rxq *rx); + + /* ## -------------------------------- ## */ + /* ## netdev flow offloading functions ## */ + /* ## -------------------------------- ## */ + + /* If a particular netdev class does not support offloading flows, + * all these function pointers must be NULL. */ + + /* Flush all offloaded flows from a netdev. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*flow_flush)(struct netdev *); + + /* Flow dumping interface. + * + * This is the back-end for the flow dumping interface described in + * dpif.h. Please read the comments there first, because this code + * closely follows it. + * + * 'flow_dump_create' is being executed in a dpif thread so there is + * no need for 'flow_dump_thread_create' implementation. + * On success returns allocated netdev_flow_dump data, on failure returns + * positive errno. */ + int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump); + int (*flow_dump_destroy)(struct netdev_flow_dump *); + + /* Returns true while there are more flows to dump. + * rbuffer is used as a temporary buffer and needs to be pre allocated + * by the caller. while there are more flows the same rbuffer should + * be provided. wbuffer is used to store dumped actions and needs to be + * pre allocated by the caller. */ + bool (*flow_dump_next)(struct netdev_flow_dump *, struct match *, + struct nlattr **actions, + struct dpif_flow_stats *stats, ovs_u128 *ufid, + struct ofpbuf *rbuffer, struct ofpbuf *wbuffer); + + /* Offload the given flow on netdev. + * To modify a flow, use the same ufid. + * actions are in netlink format, as with struct dpif_flow_put. + * info is extra info needed to offload the flow. + * Read the comments on 'struct dpif_flow_put' in dpif.h about stats. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*flow_put)(struct netdev *, struct match *, struct nlattr *actions, + size_t actions_len, const ovs_u128 *ufid, + struct offload_info *info, struct dpif_flow_stats *); + + /* Queries a flow specified by ufid on netdev. + * Fills output buffer as wbuffer in flow_dump_next. + * the buffer needs to be pre allocated. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*flow_get)(struct netdev *, struct match *, struct nlattr **actions, + const ovs_u128 *ufid, struct dpif_flow_stats *, + struct ofpbuf *wbuffer); + + /* Delete a flow specified by ufid from netdev. + * Read the comments on 'struct dpif_flow_del' in dpif.h about stats. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*flow_del)(struct netdev *, const ovs_u128 *ufid, + struct dpif_flow_stats *); + + /* Initializies the netdev flow api. */ + int (*init_flow_api)(struct netdev *); }; int netdev_register_provider(const struct netdev_class *); @@ -788,4 +857,6 @@ extern const struct netdev_class netdev_tap_class; } #endif +#define NO_OFFLOAD_API NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL + #endif /* netdev.h */ diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c new file mode 100644 index 0000000..46017d8 --- /dev/null +++ b/lib/netdev-tc-offloads.c @@ -0,0 +1,115 @@ +/* + * Copyright (c) 2016 Mellanox Technologies, Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <config.h> +#include "netdev-tc-offloads.h" +#include <errno.h> +#include <linux/if_ether.h> +#include "openvswitch/match.h" +#include "openvswitch/vlog.h" +#include "openvswitch/ofpbuf.h" +#include "openvswitch/thread.h" +#include "openvswitch/types.h" +#include "openvswitch/hmap.h" +#include "netdev-provider.h" +#include "netlink-socket.h" +#include "netlink.h" +#include "odp-netlink.h" +#include "unaligned.h" +#include "util.h" +#include "hash.h" +#include "dpif.h" +#include "tc.h" + +VLOG_DEFINE_THIS_MODULE(netdev_tc_offloads); + +int +netdev_tc_flow_flush(struct netdev *netdev OVS_UNUSED) +{ + return EOPNOTSUPP; +} + +int +netdev_tc_flow_dump_create(struct netdev *netdev, + struct netdev_flow_dump **dump_out) +{ + struct netdev_flow_dump *dump = xzalloc(sizeof *dump); + + dump->netdev = netdev_ref(netdev); + + *dump_out = dump; + + return 0; +} + +int +netdev_tc_flow_dump_destroy(struct netdev_flow_dump *dump) +{ + netdev_close(dump->netdev); + free(dump); + + return 0; +} + +bool +netdev_tc_flow_dump_next(struct netdev_flow_dump *dump OVS_UNUSED, + struct match *match OVS_UNUSED, + struct nlattr **actions OVS_UNUSED, + struct dpif_flow_stats *stats OVS_UNUSED, + ovs_u128 *ufid OVS_UNUSED, + struct ofpbuf *rbuffer OVS_UNUSED, + struct ofpbuf *wbuffer OVS_UNUSED) +{ + return false; +} + +int +netdev_tc_flow_put(struct netdev *netdev OVS_UNUSED, + struct match *match OVS_UNUSED, + struct nlattr *actions OVS_UNUSED, + size_t actions_len OVS_UNUSED, + const ovs_u128 *ufid OVS_UNUSED, + struct offload_info *info OVS_UNUSED, + struct dpif_flow_stats *stats OVS_UNUSED) +{ + return EOPNOTSUPP; +} + +int +netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED, + struct match *match OVS_UNUSED, + struct nlattr **actions OVS_UNUSED, + const ovs_u128 *ufid OVS_UNUSED, + struct dpif_flow_stats *stats OVS_UNUSED, + struct ofpbuf *buf OVS_UNUSED) +{ + return EOPNOTSUPP; +} + +int +netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, + const ovs_u128 *ufid OVS_UNUSED, + struct dpif_flow_stats *stats OVS_UNUSED) +{ + return EOPNOTSUPP; +} + +int +netdev_tc_init_flow_api(struct netdev *netdev OVS_UNUSED) +{ + return 0; +} + diff --git a/lib/netdev-tc-offloads.h b/lib/netdev-tc-offloads.h new file mode 100644 index 0000000..317347e --- /dev/null +++ b/lib/netdev-tc-offloads.h @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2016 Mellanox Technologies, Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef NETDEV_TC_OFFLOADS_H +#define NETDEV_TC_OFFLOADS_H 1 + +#include "netdev-provider.h" + +int netdev_tc_flow_flush(struct netdev *); +int netdev_tc_flow_dump_create(struct netdev *, struct netdev_flow_dump **); +int netdev_tc_flow_dump_destroy(struct netdev_flow_dump *); +bool netdev_tc_flow_dump_next(struct netdev_flow_dump *, struct match *, + struct nlattr **actions, + struct dpif_flow_stats *, + ovs_u128 *ufid, + struct ofpbuf *rbuffer, + struct ofpbuf *wbuffer); +int netdev_tc_flow_put(struct netdev *, struct match *, + struct nlattr *actions, size_t actions_len, + const ovs_u128 *, struct offload_info *, + struct dpif_flow_stats *); +int netdev_tc_flow_get(struct netdev *, struct match *, + struct nlattr **actions, const ovs_u128 *, + struct dpif_flow_stats *, struct ofpbuf *); +int netdev_tc_flow_del(struct netdev *, const ovs_u128 *, + struct dpif_flow_stats *); +int netdev_tc_init_flow_api(struct netdev *); + +#endif /* netdev-tc-offloads.h */ diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 39093e8..2cad5cb 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -847,7 +847,16 @@ get_stats(const struct netdev *netdev, struct netdev_stats *stats) NULL, /* rx_dealloc */ \ NULL, /* rx_recv */ \ NULL, /* rx_wait */ \ - NULL, /* rx_drain */ + NULL, /* rx_drain */ \ + \ + NULL, /* flow_flush */ \ + NULL, /* flow_dump_create */ \ + NULL, /* flow_dump_destroy */ \ + NULL, /* flow_dump_next */ \ + NULL, /* flow_put */ \ + NULL, /* flow_get */ \ + NULL, /* flow_del */ \ + NULL, /* init_flow_api */ #define TUNNEL_CLASS(NAME, DPIF_PORT, BUILD_HEADER, PUSH_HEADER, POP_HEADER) \ diff --git a/lib/netdev.c b/lib/netdev.c index a8d8eda..9eb37ec 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -1998,3 +1998,97 @@ netdev_reconfigure(struct netdev *netdev) ? class->reconfigure(netdev) : EOPNOTSUPP); } + +int +netdev_flow_flush(struct netdev *netdev) +{ + const struct netdev_class *class = netdev->netdev_class; + + return (class->flow_flush + ? class->flow_flush(netdev) + : EOPNOTSUPP); +} + +int +netdev_flow_dump_create(struct netdev *netdev, struct netdev_flow_dump **dump) +{ + const struct netdev_class *class = netdev->netdev_class; + + return (class->flow_dump_create + ? class->flow_dump_create(netdev, dump) + : EOPNOTSUPP); +} + +int +netdev_flow_dump_destroy(struct netdev_flow_dump *dump) +{ + const struct netdev_class *class = dump->netdev->netdev_class; + + return (class->flow_dump_destroy + ? class->flow_dump_destroy(dump) + : EOPNOTSUPP); +} + +bool +netdev_flow_dump_next(struct netdev_flow_dump *dump, + struct match *match, + struct nlattr **actions, + struct dpif_flow_stats *stats, + ovs_u128 *ufid, + struct ofpbuf *rbuffer, + struct ofpbuf *wbuffer) +{ + const struct netdev_class *class = dump->netdev->netdev_class; + + return (class->flow_dump_next + ? class->flow_dump_next(dump, match, actions, stats, ufid, + rbuffer, wbuffer) + : false); +} + +int +netdev_flow_put(struct netdev *netdev, struct match *match, + struct nlattr *actions, size_t act_len, + const ovs_u128 *ufid, struct offload_info *info, + struct dpif_flow_stats *stats) +{ + const struct netdev_class *class = netdev->netdev_class; + + return (class->flow_put + ? class->flow_put(netdev, match, actions, act_len, ufid, + info, stats) + : EOPNOTSUPP); +} + +int +netdev_flow_get(struct netdev *netdev, struct match *match, + struct nlattr **actions, const ovs_u128 *ufid, + struct dpif_flow_stats *stats, struct ofpbuf *buf) +{ + const struct netdev_class *class = netdev->netdev_class; + + return (class->flow_get + ? class->flow_get(netdev, match, actions, ufid, stats, buf) + : EOPNOTSUPP); +} + +int +netdev_flow_del(struct netdev *netdev, const ovs_u128 *ufid, + struct dpif_flow_stats *stats) +{ + const struct netdev_class *class = netdev->netdev_class; + + return (class->flow_del + ? class->flow_del(netdev, ufid, stats) + : EOPNOTSUPP); +} + +int +netdev_init_flow_api(struct netdev *netdev) +{ + const struct netdev_class *class = netdev->netdev_class; + + return (class->init_flow_api + ? class->init_flow_api(netdev) + : EOPNOTSUPP); +} diff --git a/lib/netdev.h b/lib/netdev.h index d6c07c1..17890e6 100644 --- a/lib/netdev.h +++ b/lib/netdev.h @@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct dp_packet_batch *, bool may_steal, bool concurrent_txq); void netdev_send_wait(struct netdev *, int qid); +/* Flow offloading. */ +struct offload_info { + const void *port_hmap_obj; /* To query ports info from netdev port map */ + ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */ +}; +struct netdev_flow_dump; +int netdev_flow_flush(struct netdev *); +int netdev_flow_dump_create(struct netdev *, struct netdev_flow_dump **dump); +int netdev_flow_dump_destroy(struct netdev_flow_dump *); +bool netdev_flow_dump_next(struct netdev_flow_dump *, struct match *, + struct nlattr **actions, struct dpif_flow_stats *, + ovs_u128 *ufid, struct ofpbuf *rbuffer, + struct ofpbuf *wbuffer); +int netdev_flow_put(struct netdev *, struct match *, struct nlattr *actions, + size_t actions_len, const ovs_u128 *, + struct offload_info *, struct dpif_flow_stats *); +int netdev_flow_get(struct netdev *, struct match *, struct nlattr **actions, + const ovs_u128 *, struct dpif_flow_stats *, + struct ofpbuf *wbuffer); +int netdev_flow_del(struct netdev *, const ovs_u128 *, + struct dpif_flow_stats *); +int netdev_init_flow_api(struct netdev *); + /* native tunnel APIs */ /* Structure to pass parameters required to build a tunnel header. */ struct netdev_tnl_build_header_params {