[ovs-dev] dpif-netdev: Fix comments in function headers

Message ID 1502448996-7833-1-git-send-email-antonio.fischetti@intel.com
State Accepted
Delegated to: Darrell Ball
Headers show

Commit Message

Fischetti, Antonio Aug. 11, 2017, 10:56 a.m.
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(-)

Comments

Ferriter, Cian Sept. 1, 2017, 10:45 a.m. | #1
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>
Darrell Ball Sept. 1, 2017, 9:28 p.m. | #2
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=

Patch

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,