diff mbox

[dpdk-dev] xenvirt: fix compilation after mempool changes

Message ID 1465806137-32619-1-git-send-email-olivier.matz@6wind.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Olivier Matz June 13, 2016, 8:22 a.m. UTC 
  The field elt_va_start has been removed from the mempool structure,
and it was not replaced in xenvirt.

Fix this by getting the mempool objects address by using the address of
the first memory chunk list.

Note that it won't work with mempool composed of several chunks,
but it was already the case before.

Fixes: 84121f197187 ("mempool: store memory chunks in a list")
Reported-by: Christian Ehrhard <christian.ehrhardt@canonical.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/xenvirt/rte_xen_lib.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Christian Ehrhardt June 13, 2016, 8:51 a.m. UTC | #1 
Hi Oliver,
thanks for the fast response!

It fixes the compilation issue and I totally agree to your argument of the
multi-chunk issues being out of scope for this as they never worked.
Unfortunately I lack an environment to actually test this in real-life if
we need any more follow up than this.

Acked-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>



Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Mon, Jun 13, 2016 at 10:22 AM, Olivier Matz <olivier.matz@6wind.com>
wrote:

> The field elt_va_start has been removed from the mempool structure,
> and it was not replaced in xenvirt.
>
> Fix this by getting the mempool objects address by using the address of
> the first memory chunk list.
>
> Note that it won't work with mempool composed of several chunks,
> but it was already the case before.
>
> Fixes: 84121f197187 ("mempool: store memory chunks in a list")
> Reported-by: Christian Ehrhard <christian.ehrhardt@canonical.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/xenvirt/rte_xen_lib.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/xenvirt/rte_xen_lib.c
> b/drivers/net/xenvirt/rte_xen_lib.c
> index de63cd3..997e56e 100644
> --- a/drivers/net/xenvirt/rte_xen_lib.c
> +++ b/drivers/net/xenvirt/rte_xen_lib.c
> @@ -423,6 +423,7 @@ grant_gntalloc_mbuf_pool(struct rte_mempool *mpool,
> uint32_t pg_num, uint32_t *g
>  {
>         char key_str[PATH_MAX] = {0};
>         char val_str[PATH_MAX] = {0};
> +       void *mempool_obj_va;
>
>         if (grant_node_create(pg_num, gref_arr, pa_arr, val_str,
> sizeof(val_str))) {
>                 return -1;
> @@ -437,7 +438,14 @@ grant_gntalloc_mbuf_pool(struct rte_mempool *mpool,
> uint32_t pg_num, uint32_t *g
>         if (snprintf(key_str, sizeof(key_str),
>                 DPDK_XENSTORE_PATH"%d"MEMPOOL_VA_XENSTORE_STR,
> mempool_idx) == -1)
>                 return -1;
> -       if (snprintf(val_str, sizeof(val_str), "%"PRIxPTR,
> (uintptr_t)mpool->elt_va_start) == -1)
> +       if (mp->nb_mem_chunks != 1) {
> +               RTE_LOG(ERR, PMD,
> +                       "mempool with more than 1 chunk is not
> supported\n");
> +               return -1;
> +       }
> +       mempool_obj_va = STAILQ_FIRST(&mp->mem_list)->addr;
> +       if (snprintf(val_str, sizeof(val_str), "%"PRIxPTR,
> +                       (uintptr_t)mempool_obj_va) == -1)
>                 return -1;
>         if (xenstore_write(key_str, val_str) == -1)
>                 return -1;
> --
> 2.8.0.rc3
>
>
Christian Ehrhardt June 13, 2016, 9:10 a.m. UTC | #2 
Hmm,
Hi again Oliver.

I was too fast saying yes.
I don't know what is different now but I clearly tested it wrong the first
time.
Now I get:

  CC rte_xen_lib.o
/mnt/nvme/dpdk-16-07-pre-linking/drivers/net/xenvirt/rte_xen_lib.c: In
function ‘grant_gntalloc_mbuf_pool’:
/mnt/nvme/dpdk-16-07-pre-linking/drivers/net/xenvirt/rte_xen_lib.c:441:6:
error:
‘mp’ undeclared (first use in this function)
 if (mp->nb_mem_chunks != 1) {
     ^
/mnt/nvme/dpdk-16-07-pre-linking/drivers/net/xenvirt/rte_xen_lib.c:441:6: note:
each undeclared identifier is reported only once for each function it
appears in
/mnt/nvme/dpdk-16-07-pre-linking/drivers/net/xenvirt/rte_xen_lib.c:422:46:
error:
unused parameter ‘mpool’ [-Werror=unused-parameter]
grant_gntalloc_mbuf_pool(struct rte_mempool *mpool, uint32_t pg_num,
uint32_t *gref_arr, phys_addr_t *pa_arr, int mempool_idx)
                                             ^
cc1: all warnings being treated as errors


Not too hard, changing the mp to mpool on the two places the patch has
inserted it gets it working.




Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Mon, Jun 13, 2016 at 10:51 AM, Christian Ehrhardt <
christian.ehrhardt@canonical.com> wrote:

> Hi Oliver,
> thanks for the fast response!
>
> It fixes the compilation issue and I totally agree to your argument of the
> multi-chunk issues being out of scope for this as they never worked.
> Unfortunately I lack an environment to actually test this in real-life if
> we need any more follow up than this.
>
> Acked-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>
>
>
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>
> On Mon, Jun 13, 2016 at 10:22 AM, Olivier Matz <olivier.matz@6wind.com>
> wrote:
>
>> The field elt_va_start has been removed from the mempool structure,
>> and it was not replaced in xenvirt.
>>
>> Fix this by getting the mempool objects address by using the address of
>> the first memory chunk list.
>>
>> Note that it won't work with mempool composed of several chunks,
>> but it was already the case before.
>>
>> Fixes: 84121f197187 ("mempool: store memory chunks in a list")
>> Reported-by: Christian Ehrhard <christian.ehrhardt@canonical.com>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>>  drivers/net/xenvirt/rte_xen_lib.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/xenvirt/rte_xen_lib.c
>> b/drivers/net/xenvirt/rte_xen_lib.c
>> index de63cd3..997e56e 100644
>> --- a/drivers/net/xenvirt/rte_xen_lib.c
>> +++ b/drivers/net/xenvirt/rte_xen_lib.c
>> @@ -423,6 +423,7 @@ grant_gntalloc_mbuf_pool(struct rte_mempool *mpool,
>> uint32_t pg_num, uint32_t *g
>>  {
>>         char key_str[PATH_MAX] = {0};
>>         char val_str[PATH_MAX] = {0};
>> +       void *mempool_obj_va;
>>
>>         if (grant_node_create(pg_num, gref_arr, pa_arr, val_str,
>> sizeof(val_str))) {
>>                 return -1;
>> @@ -437,7 +438,14 @@ grant_gntalloc_mbuf_pool(struct rte_mempool *mpool,
>> uint32_t pg_num, uint32_t *g
>>         if (snprintf(key_str, sizeof(key_str),
>>                 DPDK_XENSTORE_PATH"%d"MEMPOOL_VA_XENSTORE_STR,
>> mempool_idx) == -1)
>>                 return -1;
>> -       if (snprintf(val_str, sizeof(val_str), "%"PRIxPTR,
>> (uintptr_t)mpool->elt_va_start) == -1)
>> +       if (mp->nb_mem_chunks != 1) {
>> +               RTE_LOG(ERR, PMD,
>> +                       "mempool with more than 1 chunk is not
>> supported\n");
>> +               return -1;
>> +       }
>> +       mempool_obj_va = STAILQ_FIRST(&mp->mem_list)->addr;
>> +       if (snprintf(val_str, sizeof(val_str), "%"PRIxPTR,
>> +                       (uintptr_t)mempool_obj_va) == -1)
>>                 return -1;
>>         if (xenstore_write(key_str, val_str) == -1)
>>                 return -1;
>> --
>> 2.8.0.rc3
>>
>>
>
Olivier Matz June 13, 2016, 9:13 a.m. UTC | #3 
On 06/13/2016 11:10 AM, Christian Ehrhardt wrote:
> Hmm,
> Hi again Oliver.
> 
> I was too fast saying yes.
> I don't know what is different now but I clearly tested it wrong the
> first time.
> Now I get:
> 
>   CC rte_xen_lib.o
> /mnt/nvme/dpdk-16-07-pre-linking/drivers/net/xenvirt/rte_xen_lib.c:In
> function ‘grant_gntalloc_mbuf_pool’:
> /mnt/nvme/dpdk-16-07-pre-linking/drivers/net/xenvirt/rte_xen_lib.c:441:6:error:
> ‘mp’ undeclared (first use in this function)
>  if (mp->nb_mem_chunks != 1) {
>      ^
> /mnt/nvme/dpdk-16-07-pre-linking/drivers/net/xenvirt/rte_xen_lib.c:441:6:note:
> each undeclared identifier is reported only once for each function it
> appears in
> /mnt/nvme/dpdk-16-07-pre-linking/drivers/net/xenvirt/rte_xen_lib.c:422:46:error:
> unused parameter ‘mpool’ [-Werror=unused-parameter]
> grant_gntalloc_mbuf_pool(struct rte_mempool *mpool, uint32_t pg_num,
> uint32_t *gref_arr, phys_addr_t *pa_arr, int mempool_idx)
>                                              ^
> cc1: all warnings being treated as errors
> 
> 
> Not too hard, changing the mp to mpool on the two places the patch has
> inserted it gets it working.
> 

Thanks, and sorry I did not test the compilation because I don't have
the xen libraries installed.

This shows that even reading the patch several times is less efficient
than a compiler ;)

I'll send a v2 soon.
diff mbox

Patch

diff --git a/drivers/net/xenvirt/rte_xen_lib.c b/drivers/net/xenvirt/rte_xen_lib.c
index de63cd3..997e56e 100644
--- a/drivers/net/xenvirt/rte_xen_lib.c
+++ b/drivers/net/xenvirt/rte_xen_lib.c
@@ -423,6 +423,7 @@  grant_gntalloc_mbuf_pool(struct rte_mempool *mpool, uint32_t pg_num, uint32_t *g
 {
 	char key_str[PATH_MAX] = {0};
 	char val_str[PATH_MAX] = {0};
+	void *mempool_obj_va;
 
 	if (grant_node_create(pg_num, gref_arr, pa_arr, val_str, sizeof(val_str))) {
 		return -1;
@@ -437,7 +438,14 @@  grant_gntalloc_mbuf_pool(struct rte_mempool *mpool, uint32_t pg_num, uint32_t *g
 	if (snprintf(key_str, sizeof(key_str),
 		DPDK_XENSTORE_PATH"%d"MEMPOOL_VA_XENSTORE_STR, mempool_idx) == -1)
 		return -1;
-	if (snprintf(val_str, sizeof(val_str), "%"PRIxPTR, (uintptr_t)mpool->elt_va_start) == -1)
+	if (mp->nb_mem_chunks != 1) {
+		RTE_LOG(ERR, PMD,
+			"mempool with more than 1 chunk is not supported\n");
+		return -1;
+	}
+	mempool_obj_va = STAILQ_FIRST(&mp->mem_list)->addr;
+	if (snprintf(val_str, sizeof(val_str), "%"PRIxPTR,
+			(uintptr_t)mempool_obj_va) == -1)
 		return -1;
 	if (xenstore_write(key_str, val_str) == -1)
 		return -1;