diff mbox

[ovs-dev] netdev-dpdk: round up mbuf_size to cache_line_size

Message ID 20170531130812.11761-1-santosh.shukla@caviumnetworks.com
State Superseded
Delegated to: Darrell Ball
Headers show

Commit Message

santosh May 31, 2017, 1:08 p.m. UTC 
Some pmd driver(e.g: vNIC thunderx PMD) want mbuf_size to be multiple of
cache_line_size. With out this fix, Netdev-dpdk initialization would
simply fail for those drivers.

Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
---
- Tested arm64/ThunderX platform for vNIC pmd,
- Topology: phy-phy and phy-vm
- Tested x86 platform for XL710/i40e pmd.

 lib/netdev-dpdk.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

santosh June 5, 2017, 5:01 a.m. UTC | #1 
On Wednesday 31 May 2017 06:38 PM, Santosh Shukla wrote:

> Some pmd driver(e.g: vNIC thunderx PMD) want mbuf_size to be multiple of
> cache_line_size. With out this fix, Netdev-dpdk initialization would
> simply fail for those drivers.
>
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> ---
> - Tested arm64/ThunderX platform for vNIC pmd,
> - Topology: phy-phy and phy-vm
> - Tested x86 platform for XL710/i40e pmd.

Ping? Review comment/feedback?

Thanks.
santosh June 7, 2017, 1:10 p.m. UTC | #2 
Hi,

On Monday 05 June 2017 10:31 AM, santosh wrote:

> On Wednesday 31 May 2017 06:38 PM, Santosh Shukla wrote:
>
>> Some pmd driver(e.g: vNIC thunderx PMD) want mbuf_size to be multiple of
>> cache_line_size. With out this fix, Netdev-dpdk initialization would
>> simply fail for those drivers.
>>
>> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>> ---
>> - Tested arm64/ThunderX platform for vNIC pmd,
>> - Topology: phy-phy and phy-vm
>> - Tested x86 platform for XL710/i40e pmd.
> Ping? Review comment/feedback?

Pinging again? Appreciate any review comment/feedback about patch. Thanks.

> Thanks.
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff June 8, 2017, 8:42 p.m. UTC | #3 
On Wed, Jun 07, 2017 at 06:40:54PM +0530, santosh wrote:
> Hi,
> 
> On Monday 05 June 2017 10:31 AM, santosh wrote:
> 
> > On Wednesday 31 May 2017 06:38 PM, Santosh Shukla wrote:
> >
> >> Some pmd driver(e.g: vNIC thunderx PMD) want mbuf_size to be multiple of
> >> cache_line_size. With out this fix, Netdev-dpdk initialization would
> >> simply fail for those drivers.
> >>
> >> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> >> ---
> >> - Tested arm64/ThunderX platform for vNIC pmd,
> >> - Topology: phy-phy and phy-vm
> >> - Tested x86 platform for XL710/i40e pmd.
> > Ping? Review comment/feedback?
> 
> Pinging again? Appreciate any review comment/feedback about patch. Thanks.

It seems reasonable to me but I'd like to hear from someone who knows
DPDK better.
Darrell Ball June 8, 2017, 11:20 p.m. UTC | #4 
On 5/31/17, 6:08 AM, "ovs-dev-bounces@openvswitch.org on behalf of Santosh Shukla" <ovs-dev-bounces@openvswitch.org on behalf of santosh.shukla@caviumnetworks.com> wrote:

    Some pmd driver(e.g: vNIC thunderx PMD) want mbuf_size to be multiple of
    cache_line_size. With out this fix, Netdev-dpdk initialization would
    simply fail for those drivers.
    
    Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>

    ---
    - Tested arm64/ThunderX platform for vNIC pmd,
    - Topology: phy-phy and phy-vm
    - Tested x86 platform for XL710/i40e pmd.
    
     lib/netdev-dpdk.c | 7 +++++--
     1 file changed, 5 insertions(+), 2 deletions(-)
    
    diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
    index 9941f884f..1952a5cec 100644
    --- a/lib/netdev-dpdk.c
    +++ b/lib/netdev-dpdk.c
    @@ -76,9 +76,12 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
     #define MTU_TO_MAX_FRAME_LEN(mtu)   ((mtu) + ETHER_HDR_MAX_LEN)
     #define FRAME_LEN_TO_MTU(frame_len) ((frame_len)                    \
                                          - ETHER_HDR_LEN - ETHER_CRC_LEN)
    -#define MBUF_SIZE(mtu)              (MTU_TO_MAX_FRAME_LEN(mtu)      \
    +#define _MBUF_SIZE(mtu)             (MTU_TO_MAX_FRAME_LEN(mtu)      \
                                          + sizeof(struct dp_packet)     \
                                          + RTE_PKTMBUF_HEADROOM)
    +#define MBUF_SIZE(mtu)              ROUND_UP((_MBUF_SIZE(mtu)),     \
    +                                             RTE_CACHE_LINE_SIZE)

Why not just add ROUND_UP to the existing MBUF_SIZE macro; I think it is
more straightforward and there are probably enough ‘MTU’ macros
already.


    +
     #define NETDEV_DPDK_MBUF_ALIGN      1024
     #define NETDEV_DPDK_MAX_PKT_LEN     9728
     
    @@ -495,7 +498,7 @@ dpdk_mp_create(int socket_id, int mtu)
                                               MP_CACHE_SZ,
                                               sizeof (struct dp_packet)
                                                      - sizeof (struct rte_mbuf),
    -                                          MBUF_SIZE(mtu)
    +                                          MBUF_SIZE(dmp->mtu)

This change does not seem necessary and distracts from the point
of the patch.

                                                      - sizeof(struct dp_packet),
                                               socket_id);
             if (dmp->mp) {
    -- 
    2.11.0
    
    _______________________________________________
    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=1tNyJJHbYGc7-BZ-RPxJoTCjeqEwQs-1EhjxLywTsfo&s=oGUJD3e3CuDW6phStK-eGR-rFu7ldsm3VeSL3Zd7vcg&e=
santosh June 9, 2017, 4:49 a.m. UTC | #5 
Hi Darrell,

On Friday 09 June 2017 04:50 AM, Darrell Ball wrote:

>
> On 5/31/17, 6:08 AM, "ovs-dev-bounces@openvswitch.org on behalf of Santosh Shukla" <ovs-dev-bounces@openvswitch.org on behalf of santosh.shukla@caviumnetworks.com> wrote:
>
>     Some pmd driver(e.g: vNIC thunderx PMD) want mbuf_size to be multiple of
>     cache_line_size. With out this fix, Netdev-dpdk initialization would
>     simply fail for those drivers.
>     
>     Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>     ---
>     - Tested arm64/ThunderX platform for vNIC pmd,
>     - Topology: phy-phy and phy-vm
>     - Tested x86 platform for XL710/i40e pmd.
>     
>      lib/netdev-dpdk.c | 7 +++++--
>      1 file changed, 5 insertions(+), 2 deletions(-)
>     
>     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>     index 9941f884f..1952a5cec 100644
>     --- a/lib/netdev-dpdk.c
>     +++ b/lib/netdev-dpdk.c
>     @@ -76,9 +76,12 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>      #define MTU_TO_MAX_FRAME_LEN(mtu)   ((mtu) + ETHER_HDR_MAX_LEN)
>      #define FRAME_LEN_TO_MTU(frame_len) ((frame_len)                    \
>                                           - ETHER_HDR_LEN - ETHER_CRC_LEN)
>     -#define MBUF_SIZE(mtu)              (MTU_TO_MAX_FRAME_LEN(mtu)      \
>     +#define _MBUF_SIZE(mtu)             (MTU_TO_MAX_FRAME_LEN(mtu)      \
>                                           + sizeof(struct dp_packet)     \
>                                           + RTE_PKTMBUF_HEADROOM)
>     +#define MBUF_SIZE(mtu)              ROUND_UP((_MBUF_SIZE(mtu)),     \
>     +                                             RTE_CACHE_LINE_SIZE)
>
> Why not just add ROUND_UP to the existing MBUF_SIZE macro; I think it is
> more straightforward and there are probably enough ‘MTU’ macros
> already.

ok, Will do in v2.

>
>     +
>      #define NETDEV_DPDK_MBUF_ALIGN      1024
>      #define NETDEV_DPDK_MAX_PKT_LEN     9728
>      
>     @@ -495,7 +498,7 @@ dpdk_mp_create(int socket_id, int mtu)
>                                                MP_CACHE_SZ,
>                                                sizeof (struct dp_packet)
>                                                       - sizeof (struct rte_mbuf),
>     -                                          MBUF_SIZE(mtu)
>     +                                          MBUF_SIZE(dmp->mtu)
>
> This change does not seem necessary and distracts from the point
> of the patch.

Ok, Will stick to 'mtu' instead of dmp->mtu. Although, higher up in function mtu
assigned to dmp->mtu, so it made sense to me to use 'dmp->mtu'. But I agree
with your argument. Perhaps should go as separate cleanup patch.

Will do in v2.

Thanks for review feedback.

>                                                       - sizeof(struct dp_packet),
>                                                socket_id);
>              if (dmp->mp) {
>     -- 
>     2.11.0
>     
>     _______________________________________________
>     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=1tNyJJHbYGc7-BZ-RPxJoTCjeqEwQs-1EhjxLywTsfo&s=oGUJD3e3CuDW6phStK-eGR-rFu7ldsm3VeSL3Zd7vcg&e= 
>     
>
diff mbox

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 9941f884f..1952a5cec 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -76,9 +76,12 @@  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
 #define MTU_TO_MAX_FRAME_LEN(mtu)   ((mtu) + ETHER_HDR_MAX_LEN)
 #define FRAME_LEN_TO_MTU(frame_len) ((frame_len)                    \
                                      - ETHER_HDR_LEN - ETHER_CRC_LEN)
-#define MBUF_SIZE(mtu)              (MTU_TO_MAX_FRAME_LEN(mtu)      \
+#define _MBUF_SIZE(mtu)             (MTU_TO_MAX_FRAME_LEN(mtu)      \
                                      + sizeof(struct dp_packet)     \
                                      + RTE_PKTMBUF_HEADROOM)
+#define MBUF_SIZE(mtu)              ROUND_UP((_MBUF_SIZE(mtu)),     \
+                                             RTE_CACHE_LINE_SIZE)
+
 #define NETDEV_DPDK_MBUF_ALIGN      1024
 #define NETDEV_DPDK_MAX_PKT_LEN     9728
 
@@ -495,7 +498,7 @@  dpdk_mp_create(int socket_id, int mtu)
                                           MP_CACHE_SZ,
                                           sizeof (struct dp_packet)
                                                  - sizeof (struct rte_mbuf),
-                                          MBUF_SIZE(mtu)
+                                          MBUF_SIZE(dmp->mtu)
                                                  - sizeof(struct dp_packet),
                                           socket_id);
         if (dmp->mp) {