[dpdk-dev] Missing an increment on vq->log_cache_nb_elem ?

[dpdk-dev] Missing an increment on vq->log_cache_nb_elem ?

[HePeng]

Hi, 


In the latest dpdk master branch, in the function *vhost_log_cache_page*:


static __rte_always_inline void
vhost_log_cache_page(struct virtio_net *dev, struct vhost_virtqueue *vq,
			uint64_t page)
{
	uint32_t bit_nr = page % (sizeof(unsigned long) << 3);
	uint32_t offset = page / (sizeof(unsigned long) << 3);
	int i;

	for (i = 0; i < vq->log_cache_nb_elem; i++) {
		struct log_cache_entry *elem = vq->log_cache + i;

		if (elem->offset == offset) {
			elem->val |= (1UL << bit_nr);
			return;
		}
	}

	if (unlikely(i >= VHOST_LOG_CACHE_NR)) {
		/*
		 * No more room for a new log cache entry,
		 * so write the dirty log map directly.
		 */
		rte_smp_wmb();
		vhost_log_page((uint8_t *)(uintptr_t)dev->log_base, page);

		return;
	}

	vq->log_cache[i].offset = offset;
	vq->log_cache[i].val = (1UL << bit_nr);
}

Did it just miss an increment on vq->log_cache_nb_elem ?

Re: [dpdk-dev] Missing an increment on vq->log_cache_nb_elem ?

[Ilya Maximets]

> Hi, 
> 
> 
> In the latest dpdk master branch, in the function *vhost_log_cache_page*:
> 
> 
> static __rte_always_inline void
> vhost_log_cache_page(struct virtio_net *dev, struct vhost_virtqueue *vq,
> 			uint64_t page)
> {
> 	uint32_t bit_nr = page % (sizeof(unsigned long) << 3);
> 	uint32_t offset = page / (sizeof(unsigned long) << 3);
> 	int i;
> 
> 	for (i = 0; i < vq->log_cache_nb_elem; i++) {
> 		struct log_cache_entry *elem = vq->log_cache + i;
> 
> 		if (elem->offset == offset) {
> 			elem->val |= (1UL << bit_nr);
> 			return;
> 		}
> 	}
> 
> 	if (unlikely(i >= VHOST_LOG_CACHE_NR)) {
> 		/*
> 		 * No more room for a new log cache entry,
> 		 * so write the dirty log map directly.
> 		 */
> 		rte_smp_wmb();
> 		vhost_log_page((uint8_t *)(uintptr_t)dev->log_base, page);
> 
> 		return;
> 	}
> 
> 	vq->log_cache[i].offset = offset;
> 	vq->log_cache[i].val = (1UL << bit_nr);
> }
> 
> Did it just miss an increment on vq->log_cache_nb_elem ?

Hi. Thanks for pointing that.

I looked through the code and I see no updates of 'vq->log_cache_nb_elem'.
Looks like it always equal to initial zero value.

Maxime, I'm afraid that this means that currently pages logging is not
performed at all. Could you please check?

Best regards, Ilya Maximets.

Re: [dpdk-dev] Missing an increment on vq->log_cache_nb_elem ?

[Maxime Coquelin]

Hi,

On 06/15/2018 02:32 PM, Ilya Maximets wrote:
>> Hi,
>>
>>
>> In the latest dpdk master branch, in the function *vhost_log_cache_page*:
>>
>>
>> static __rte_always_inline void
>> vhost_log_cache_page(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> 			uint64_t page)
>> {
>> 	uint32_t bit_nr = page % (sizeof(unsigned long) << 3);
>> 	uint32_t offset = page / (sizeof(unsigned long) << 3);
>> 	int i;
>>
>> 	for (i = 0; i < vq->log_cache_nb_elem; i++) {
>> 		struct log_cache_entry *elem = vq->log_cache + i;
>>
>> 		if (elem->offset == offset) {
>> 			elem->val |= (1UL << bit_nr);
>> 			return;
>> 		}
>> 	}
>>
>> 	if (unlikely(i >= VHOST_LOG_CACHE_NR)) {
>> 		/*
>> 		 * No more room for a new log cache entry,
>> 		 * so write the dirty log map directly.
>> 		 */
>> 		rte_smp_wmb();
>> 		vhost_log_page((uint8_t *)(uintptr_t)dev->log_base, page);
>>
>> 		return;
>> 	}
>>
>> 	vq->log_cache[i].offset = offset;
>> 	vq->log_cache[i].val = (1UL << bit_nr);
>> }
>>
>> Did it just miss an increment on vq->log_cache_nb_elem ?
> 
> Hi. Thanks for pointing that.
> 
> I looked through the code and I see no updates of 'vq->log_cache_nb_elem'.
> Looks like it always equal to initial zero value.
> 
> Maxime, I'm afraid that this means that currently pages logging is not
> performed at all. Could you please check?
> 
> Best regards, Ilya Maximets.
> 

Thanks Ilya for the heads-up and HePeng for reporting the issue.
It is broken indeed... We didn't noticed corruption when running
our non-reg tests, I guess they have to be improved.

I will post a patch shortly to fix this.

HePeng, how does your name spells in the form "$firstname $lastname"?
I would need this info to credit you for reporting the issue.

Regards,
Maxime

Re: [dpdk-dev] Missing an increment on vq->log_cache_nb_elem ?

[HePeng]

Hi, Maxime,

  My name is Peng He. 

  Thanks. 

> 在 2018年6月15日，下午9:24，Maxime Coquelin <maxime.coquelin@redhat.com> 写道：
> 
> Hi,
> 
> On 06/15/2018 02:32 PM, Ilya Maximets wrote:
>>> Hi,
>>> 
>>> 
>>> In the latest dpdk master branch, in the function *vhost_log_cache_page*:
>>> 
>>> 
>>> static __rte_always_inline void
>>> vhost_log_cache_page(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>> 			uint64_t page)
>>> {
>>> 	uint32_t bit_nr = page % (sizeof(unsigned long) << 3);
>>> 	uint32_t offset = page / (sizeof(unsigned long) << 3);
>>> 	int i;
>>> 
>>> 	for (i = 0; i < vq->log_cache_nb_elem; i++) {
>>> 		struct log_cache_entry *elem = vq->log_cache + i;
>>> 
>>> 		if (elem->offset == offset) {
>>> 			elem->val |= (1UL << bit_nr);
>>> 			return;
>>> 		}
>>> 	}
>>> 
>>> 	if (unlikely(i >= VHOST_LOG_CACHE_NR)) {
>>> 		/*
>>> 		 * No more room for a new log cache entry,
>>> 		 * so write the dirty log map directly.
>>> 		 */
>>> 		rte_smp_wmb();
>>> 		vhost_log_page((uint8_t *)(uintptr_t)dev->log_base, page);
>>> 
>>> 		return;
>>> 	}
>>> 
>>> 	vq->log_cache[i].offset = offset;
>>> 	vq->log_cache[i].val = (1UL << bit_nr);
>>> }
>>> 
>>> Did it just miss an increment on vq->log_cache_nb_elem ?
>> Hi. Thanks for pointing that.
>> I looked through the code and I see no updates of 'vq->log_cache_nb_elem'.
>> Looks like it always equal to initial zero value.
>> Maxime, I'm afraid that this means that currently pages logging is not
>> performed at all. Could you please check?
>> Best regards, Ilya Maximets.
> 
> Thanks Ilya for the heads-up and HePeng for reporting the issue.
> It is broken indeed... We didn't noticed corruption when running
> our non-reg tests, I guess they have to be improved.
> 
> I will post a patch shortly to fix this.
> 
> HePeng, how does your name spells in the form "$firstname $lastname"?
> I would need this info to credit you for reporting the issue.
> 
> Regards,
> Maxime