Re: [PATCH 1/7] vhost: improve IOTLB logs

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi David,

On 1/4/22 15:44, David Marchand wrote:
> On Thu, Dec 23, 2021 at 9:37 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch adds IOTLB mempool name when logging debug
>> or error messages, and also prepends the socket path.
>> to all the logs.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/vhost/iotlb.c      | 26 +++++++++++++++-----------
>>   lib/vhost/iotlb.h      | 10 +++++-----
>>   lib/vhost/vhost.c      |  2 +-
>>   lib/vhost/vhost_user.c |  2 +-
>>   4 files changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
>> index 82bdb84526..e9e1ede7a4 100644
>> --- a/lib/vhost/iotlb.c
>> +++ b/lib/vhost/iotlb.c
>> @@ -62,7 +62,7 @@ vhost_user_iotlb_pending_miss(struct vhost_virtqueue *vq, uint64_t iova,
>>   }
>>
>>   void
>> -vhost_user_iotlb_pending_insert(struct vhost_virtqueue *vq,
>> +vhost_user_iotlb_pending_insert(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>                                  uint64_t iova, uint8_t perm)
>>   {
>>          struct vhost_iotlb_entry *node;
>> @@ -70,14 +70,16 @@ vhost_user_iotlb_pending_insert(struct vhost_virtqueue *vq,
>>
>>          ret = rte_mempool_get(vq->iotlb_pool, (void **)&node);
>>          if (ret) {
>> -               VHOST_LOG_CONFIG(DEBUG, "IOTLB pool empty, clear entries\n");
>> +               VHOST_LOG_CONFIG(DEBUG, "(%s) IOTLB pool %s empty, clear entries\n",
>> +                               dev->ifname, vq->iotlb_pool->name);
>>                  if (!TAILQ_EMPTY(&vq->iotlb_pending_list))
>>                          vhost_user_iotlb_pending_remove_all(vq);
>>                  else
>>                          vhost_user_iotlb_cache_random_evict(vq);
>>                  ret = rte_mempool_get(vq->iotlb_pool, (void **)&node);
>>                  if (ret) {
>> -                       VHOST_LOG_CONFIG(ERR, "IOTLB pool still empty, failure\n");
>> +                       VHOST_LOG_CONFIG(ERR, "(%s) IOTLB pool %s still empty, failure\n",
>> +                                       dev->ifname, vq->iotlb_pool->name);
>>                          return;
>>                  }
>>          }
>> @@ -156,22 +158,25 @@ vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
>>   }
>>
>>   void
>> -vhost_user_iotlb_cache_insert(struct vhost_virtqueue *vq, uint64_t iova,
>> -                               uint64_t uaddr, uint64_t size, uint8_t perm)
>> +vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> +                               uint64_t iova, uint64_t uaddr,
>> +                               uint64_t size, uint8_t perm)
>>   {
>>          struct vhost_iotlb_entry *node, *new_node;
>>          int ret;
>>
>>          ret = rte_mempool_get(vq->iotlb_pool, (void **)&new_node);
>>          if (ret) {
>> -               VHOST_LOG_CONFIG(DEBUG, "IOTLB pool empty, clear entries\n");
>> +               VHOST_LOG_CONFIG(DEBUG, "(%s) IOTLB pool %s empty, clear entries\n",
>> +                               dev->ifname, vq->iotlb_pool->name);
> 
> We have the same logs in two different paths
> (vhost_user_iotlb_pending_insert and vhost_user_iotlb_cache_insert).
> It would probably help when debugging to have separate messages.
> 
> This could be added later, since this current patch is about prefixing
> with the socket path.

Makes sense, I'm adding a new patch on top of the series to address
that.

> 
>>                  if (!TAILQ_EMPTY(&vq->iotlb_list))
>>                          vhost_user_iotlb_cache_random_evict(vq);
>>                  else
>>                          vhost_user_iotlb_pending_remove_all(vq);
>>                  ret = rte_mempool_get(vq->iotlb_pool, (void **)&new_node);
>>                  if (ret) {
>> -                       VHOST_LOG_CONFIG(ERR, "IOTLB pool still empty, failure\n");
>> +                       VHOST_LOG_CONFIG(ERR, "(%s) IOTLB pool %s still empty, failure\n",
>> +                                       dev->ifname, vq->iotlb_pool->name);
>>                          return;
>>                  }
>>          }
>> @@ -311,7 +316,7 @@ vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
>>
>>          snprintf(pool_name, sizeof(pool_name), "iotlb_%u_%d_%d",
>>                          getpid(), dev->vid, vq_index);
>> -       VHOST_LOG_CONFIG(DEBUG, "IOTLB cache name: %s\n", pool_name);
>> +       VHOST_LOG_CONFIG(DEBUG, "(%s) IOTLB cache name: %s\n", dev->ifname, pool_name);
>>
>>          /* If already created, free it and recreate */
>>          vq->iotlb_pool = rte_mempool_lookup(pool_name);
>> @@ -324,9 +329,8 @@ vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
>>                          RTE_MEMPOOL_F_NO_CACHE_ALIGN |
>>                          RTE_MEMPOOL_F_SP_PUT);
>>          if (!vq->iotlb_pool) {
>> -               VHOST_LOG_CONFIG(ERR,
>> -                               "Failed to create IOTLB cache pool (%s)\n",
>> -                               pool_name);
>> +               VHOST_LOG_CONFIG(ERR, "(%s) Failed to create IOTLB cache pool (%s)\n",
> 
> I'd make this log consistent with the previous log and remove the ()
> around the pool name.

Done.

> 
>> +                               dev->ifname, pool_name);
>>                  return -1;
>>          }
>>
> 
> The rest lgtm.
> 
> 
> 

Thanks,
Maxime