Re: Question on ptr_ring linux header

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jan 15, 2019 at 12:33:28PM +0800, fei phung wrote:
> Hi netdev mailing list and Michael,
> 
> I am having problem getting proper ptr_ring operation where one
> ptr_ring entry (val1=2 for item_recv_pop) is missing from the void **
> queue
> 
> Did I initialize the ring pointers (ptr_ring_init()) correctly ?
> 
> See the following for more context:
> 
> https://i.imgur.com/xWJOH1G.png
> https://gist.github.com/promach/7716ee8addcaa33fda140d74d1ad94d6#file-riffa_driver-c-L663

Didn't read in depth but
this one seems to poke at the internal producer index for its own
purposes. That's not something I expected when I built ptr_ring so
I don't know how well that will work. You might want to
try and come up with a reasonable API for that instead.


> https://gist.github.com/promach/65e9331d55a43a2815239430a28e29c6#file-circ_ring-c-L44
> 


Hard to say for sure but things like that

inline int push_circ_queue(struct ptr_ring * buffer, struct item * item_push)
{ 
	if (!ptr_ring_full_any(buffer)) // if (not full)
	{
		DEBUG_MSG(KERN_INFO "Pushing %u and %u\n", item_push->val1, item_push->val2);

		/* insert one item into the buffer */
	    ptr_ring_produce_any(buffer, item_push);


are a waste, and they can be racy with multiple producers.


Just call ptr_ring_produce_any and it will tell you whether it
could produce.


same here:

inline int pop_circ_queue(struct ptr_ring * buffer, struct item * item_pop)
{
	if (!ptr_ring_empty_any(buffer)) // if (not empty)
	{
		DEBUG_MSG(KERN_INFO "Before pop, head = %u , tail = %u\n", buffer->consumer_head, buffer->consumer_tail);

		/* extract one item struct containing two unsigned integers from the buffer */
		*item_pop = *((struct item *)ptr_ring_consume_any(buffer));

		DEBUG_MSG(KERN_INFO "val1 = %u , val2 = %u\n", item_pop->val1, item_pop->val2);	    

		DEBUG_MSG(KERN_INFO "After pop, head = %u , tail = %u\n", buffer->consumer_head, buffer->consumer_tail);

		return 0;
	}

	else return 1; // empty, nothing to pop from the ring
}

racy if there are multiple consumers.
just call ptr_ring_consume_any.

And it seems to leak the memory the pointer to which you
have consumed - although it's possible it's freed elsewhere -
I wouldn't know.



> struct ptr_ring * init_circ_queue(int len)
> {
>         struct ptr_ring * q;
> 
>         q = kzalloc(sizeof(struct ptr_ring), GFP_KERNEL);
>         if (q == NULL) {
>                 DEBUG_MSG(KERN_ERR "Not enough memory to allocate ptr_ring");
>                 return NULL;
>         }
> 
>         // creates an array of length 'len' where each array location
> can store a struct * item
>         if(ptr_ring_init(q, len, GFP_KERNEL) != 0) {
>                 DEBUG_MSG(KERN_ERR "Not enough memory to allocate
> ptr_ring array");
>                 return NULL;
>         }
> 
>         return q;
> }
> 


This part looks good.


> 
>                 while ((nomsg = pop_circ_queue(sc->recv[chnl]->msgs,
> &item_recv_pop))) {
>                         prepare_to_wait(&sc->recv[chnl]->waitq, &wait,
> TASK_INTERRUPTIBLE);
>                         // Another check before we schedule.
>                         if ((nomsg =
> pop_circ_queue(sc->recv[chnl]->msgs, &item_recv_pop)))
>                                 tymeout = schedule_timeout(tymeout);
>                         finish_wait(&sc->recv[chnl]->waitq, &wait);
>                         if (signal_pending(current)) {
>                                 free_sg_buf(sc, sc->recv[chnl]->sg_map_0);
>                                 free_sg_buf(sc, sc->recv[chnl]->sg_map_1);
>                                 return -ERESTARTSYS;
>                         }
>                         if (!nomsg)
>                                 break;
>                         if (tymeout == 0) {
>                                 printk(KERN_ERR "riffa: fpga:%d
> chnl:%d, recv timed out\n", sc->id, chnl);
>                                 /*free_sg_buf(sc, sc->recv[chnl]->sg_map_0);
>                                 free_sg_buf(sc, sc->recv[chnl]->sg_map_1);
>                                 return (unsigned int)(recvd>>2);*/
>                         }
>                 }
>                 tymeout = tymeouto;
>                 msg_type = item_recv_pop.val1;
>                 msg = item_recv_pop.val2;
>                 DEBUG_MSG(KERN_INFO "recv msg_type: %u\n", msg_type);
> 
> 
> Regards,
> Phung