Message ID | 1502448996-7833-1-git-send-email-antonio.fischetti@intel.com |
---|---|
State | Accepted |
Delegated to: | Darrell Ball |
Headers | show |
Hi Antonio, > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of antonio.fischetti@intel.com > Sent: 11 August 2017 11:57 > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH] dpif-netdev: Fix comments in function headers > > Fix comments for emc_processing and dp_netdev_input__ regarding > md_is_valid. > > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> > --- > lib/dpif-netdev.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index e2cd931..5c5c20e > 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4660,8 +4660,11 @@ dp_netdev_queue_batches(struct dp_packet > *pkt, > * The function returns the number of packets that needs to be processed in > the > * 'packets' array (they have been moved to the beginning of the vector). > * > - * If 'md_is_valid' is false, the metadata in 'packets' is not valid and must > - * be initialized by this function using 'port_no'. > + * For performance reasons a caller may choose not to initialize the > + metadata > + * in 'packets_'. If 'md_is_valid' is false, the metadata in 'packets' > + * is not valid and must be initialized by this function using 'port_no'. > + * If 'md_is_valid' is true, the metadata is already valid and 'port_no' > + * will be ignored. > */ I like your explanation more, stating why the metadata might not be initialized. One nit for the above: if I wrap lines at 79 characters according to the CodingStyle.rst, the above can be changed to: diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 8b33bd3..73e6067 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4859,10 +4859,10 @@ dp_netdev_queue_batches(struct dp_packet *pkt, * 'packets' array (they have been moved to the beginning of the vector). * * For performance reasons a caller may choose not to initialize the metadata - * in 'packets_'. If 'md_is_valid' is false, the metadata in 'packets' - * is not valid and must be initialized by this function using 'port_no'. - * If 'md_is_valid' is true, the metadata is already valid and 'port_no' - * will be ignored. + * in 'packets_'. If 'md_is_valid' is false, the metadata in 'packets' is not + * valid and must be initialized by this function using 'port_no'. If + * 'md_is_valid' is true, the metadata is already valid and 'port_no' will be + * ignored. */ static inline size_t emc_processing(struct dp_netdev_pmd_thread *pmd, > static inline size_t > emc_processing(struct dp_netdev_pmd_thread *pmd, @@ -4894,10 > +4897,8 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd, > > /* Packets enter the datapath from a port (or from recirculation) here. > * > - * For performance reasons a caller may choose not to initialize the > metadata > - * in 'packets': in this case 'mdinit' is false and this function needs to > - * initialize it using 'port_no'. If the metadata in 'packets' is already > - * valid, 'md_is_valid' must be true and 'port_no' will be ignored. */ > + * When 'md_is_valid' is true the metadata in 'packets' are already valid. > + * When false the metadata in 'packets' need to be initialized. */ This makes more sense and is more succinct. I'm not sure what the 'mdinit' was in reference to before. > static void > dp_netdev_input__(struct dp_netdev_pmd_thread *pmd, > struct dp_packet_batch *packets, > -- Overall, it looks good to me. Acked-by: Cian Ferriter <cian.ferriter@intel.com>
I applied the patch to dpdk_merge here https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_darball_ovs_commits_dpdk-5Fmerge&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=A2_FCacqbp2moAo3HGFlTuxsjONUGhlN42OBcAuQQ6w&s=b6btPKhgvOFr2GOUYvktND6kaC6jc3fXI-mXfvNgXOU&e= On 9/1/17, 3:45 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ferriter, Cian" <ovs-dev-bounces@openvswitch.org on behalf of cian.ferriter@intel.com> wrote: Hi Antonio, > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of antonio.fischetti@intel.com > Sent: 11 August 2017 11:57 > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH] dpif-netdev: Fix comments in function headers > > Fix comments for emc_processing and dp_netdev_input__ regarding > md_is_valid. > > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> > --- > lib/dpif-netdev.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index e2cd931..5c5c20e > 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4660,8 +4660,11 @@ dp_netdev_queue_batches(struct dp_packet > *pkt, > * The function returns the number of packets that needs to be processed in > the > * 'packets' array (they have been moved to the beginning of the vector). > * > - * If 'md_is_valid' is false, the metadata in 'packets' is not valid and must > - * be initialized by this function using 'port_no'. > + * For performance reasons a caller may choose not to initialize the > + metadata > + * in 'packets_'. If 'md_is_valid' is false, the metadata in 'packets' > + * is not valid and must be initialized by this function using 'port_no'. > + * If 'md_is_valid' is true, the metadata is already valid and 'port_no' > + * will be ignored. > */ I like your explanation more, stating why the metadata might not be initialized. One nit for the above: if I wrap lines at 79 characters according to the CodingStyle.rst, the above can be changed to: diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 8b33bd3..73e6067 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4859,10 +4859,10 @@ dp_netdev_queue_batches(struct dp_packet *pkt, * 'packets' array (they have been moved to the beginning of the vector). * * For performance reasons a caller may choose not to initialize the metadata - * in 'packets_'. If 'md_is_valid' is false, the metadata in 'packets' - * is not valid and must be initialized by this function using 'port_no'. - * If 'md_is_valid' is true, the metadata is already valid and 'port_no' - * will be ignored. + * in 'packets_'. If 'md_is_valid' is false, the metadata in 'packets' is not + * valid and must be initialized by this function using 'port_no'. If + * 'md_is_valid' is true, the metadata is already valid and 'port_no' will be + * ignored. */ static inline size_t emc_processing(struct dp_netdev_pmd_thread *pmd, > static inline size_t > emc_processing(struct dp_netdev_pmd_thread *pmd, @@ -4894,10 > +4897,8 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd, > > /* Packets enter the datapath from a port (or from recirculation) here. > * > - * For performance reasons a caller may choose not to initialize the > metadata > - * in 'packets': in this case 'mdinit' is false and this function needs to > - * initialize it using 'port_no'. If the metadata in 'packets' is already > - * valid, 'md_is_valid' must be true and 'port_no' will be ignored. */ > + * When 'md_is_valid' is true the metadata in 'packets' are already valid. > + * When false the metadata in 'packets' need to be initialized. */ This makes more sense and is more succinct. I'm not sure what the 'mdinit' was in reference to before. > static void > dp_netdev_input__(struct dp_netdev_pmd_thread *pmd, > struct dp_packet_batch *packets, > -- Overall, it looks good to me. Acked-by: Cian Ferriter <cian.ferriter@intel.com> _______________________________________________ dev mailing list dev@openvswitch.org https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=ehDE1onzxVuCGoUFeFaHOni2PNBoBmYWYTh7NIihX9M&s=QEl59lwKvckL_VG1slNe1CPubqhfzkZ7CaPHU9g2k8U&e=
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index e2cd931..5c5c20e 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4660,8 +4660,11 @@ dp_netdev_queue_batches(struct dp_packet *pkt, * The function returns the number of packets that needs to be processed in the * 'packets' array (they have been moved to the beginning of the vector). * - * If 'md_is_valid' is false, the metadata in 'packets' is not valid and must - * be initialized by this function using 'port_no'. + * For performance reasons a caller may choose not to initialize the metadata + * in 'packets_'. If 'md_is_valid' is false, the metadata in 'packets' + * is not valid and must be initialized by this function using 'port_no'. + * If 'md_is_valid' is true, the metadata is already valid and 'port_no' + * will be ignored. */ static inline size_t emc_processing(struct dp_netdev_pmd_thread *pmd, @@ -4894,10 +4897,8 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd, /* Packets enter the datapath from a port (or from recirculation) here. * - * For performance reasons a caller may choose not to initialize the metadata - * in 'packets': in this case 'mdinit' is false and this function needs to - * initialize it using 'port_no'. If the metadata in 'packets' is already - * valid, 'md_is_valid' must be true and 'port_no' will be ignored. */ + * When 'md_is_valid' is true the metadata in 'packets' are already valid. + * When false the metadata in 'packets' need to be initialized. */ static void dp_netdev_input__(struct dp_netdev_pmd_thread *pmd, struct dp_packet_batch *packets,
Fix comments for emc_processing and dp_netdev_input__ regarding md_is_valid. Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> --- lib/dpif-netdev.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)