[dpdk-dev] [PATCH v2 2/6] vhost: introduce vhost_log_write

[Xie, Huawei]

On 12/22/2015 11:03 AM, Yuanhan Liu wrote:
> On Tue, Dec 22, 2015 at 02:45:52AM +0000, Xie, Huawei wrote:
>>>>> +static inline void __attribute__((always_inline))
>>>>> +vhost_log_write(struct virtio_net *dev, uint64_t addr, uint64_t len)
>>>>> +{
>>>>> +	uint64_t page;
>>>>> +
>>>> Before we log, we need memory barrier to make sure updates are in place.
>>>>> +	if (likely(((dev->features & (1ULL << VHOST_F_LOG_ALL)) == 0) ||
>>>>> +		   !dev->log_base || !len))
>>>>> +		return;
>>> Put a memory barrier inside set_features()?
>>>
>>> I see no var dependence here, why putting a barrier then? We are
>>> accessing and modifying same var, doesn't the cache MESI protocol
>>> will get rid of your concerns?
>> This fence isn't about feature var. It is to ensure that updates to the
>> guest buffer are committed before the logging.
> Oh.., I was thinking you were talking about the "dev->features" field
> concurrent access and modify you mentioned from V1.
>
>> For IA strong memory model, compiler barrier is enough. For other weak
>> memory model, fence is required.
>>>>> +
>>>>> +	if (unlikely(dev->log_size < ((addr + len - 1) / VHOST_LOG_PAGE / 8)))
>>>>> +		return;
> So that I should put a "rte_mb()" __here__?
>
> 	--yliu

I find that we already have the arch dependent version of rte_smp_wmb()
        --huawei
>>>>> +
>>>>> +	page = addr / VHOST_LOG_PAGE;
>>>>> +	while (page * VHOST_LOG_PAGE < addr + len) {
>>>> Let us have a page_end var to make the code simpler?
>>> Could do that.
>>>
>>>
>>>>> +		vhost_log_page((uint8_t *)(uintptr_t)dev->log_base, page);
>>>>> +		page += VHOST_LOG_PAGE;
>>>> page += 1?
>>> Oops, right.
>>>
>>> 	--yliu
>>>
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +
>>>>>  /**
>>>>>   *  Disable features in feature_mask. Returns 0 on success.
>>>>>   */