Re: Question on ptr_ring linux header

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

On Wed, Jan 16, 2019 at 01:10:31AM +0800, fei phung wrote:
> Hi,
> 
> 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
> }
> 
> > https://gist.github.com/promach/65e9331d55a43a2815239430a28e29c6#file-circ_ring-c-L44
> > 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 will definitely just call ptr_ring_consume_any() without ptr_ring_empty_any()
> 
> Which exact line has memory leak ?
> Are you referring to   struct item * item_pop   ?

yes:

                  *item_pop = *((struct item *)ptr_ring_consume_any(buffer));

seems to discard the pointer returned.

> 
> 
> // TX (PC receive) scatter gather buffer is read.
> if (vect & (1<<((5*i)+1))) {
>         recv = 1;
> 
>         item_recv_push[sc->recv[chnl]->msgs->producer].val1 = EVENT_SG_BUF_READ;
>         item_recv_push[sc->recv[chnl]->msgs->producer].val2 = 0;
> 
>         // Keep track so the thread can handle this.
>         if (push_circ_queue(sc->recv[chnl]->msgs,
> &item_recv_push[sc->recv[chnl]->msgs->producer])) {
>                 printk(KERN_ERR "riffa: fpga:%d chnl:%d, recv sg buf
> read msg queue full\n", sc->id, chnl);
>         }
>         DEBUG_MSG(KERN_INFO "riffa: fpga:%d chnl:%d, recv sg buf
> read\n", sc->id, chnl);
> }
> 
> // TX (PC receive) transaction done.
> if (vect & (1<<((5*i)+2))) {
>         recv = 1;
> 
>         item_recv_push[sc->recv[chnl]->msgs->producer].val1 = EVENT_TXN_DONE;
>         item_recv_push[sc->recv[chnl]->msgs->producer].val2 = len;
> 
>         // Read the transferred amount.
>         len = read_reg(sc, CHNL_REG(chnl, TX_TNFR_LEN_REG_OFF));
>         // Notify the thread.
>         if (push_circ_queue(sc->recv[chnl]->msgs,
> &item_recv_push[sc->recv[chnl]->msgs->producer])) {
>                 printk(KERN_ERR "riffa: fpga:%d chnl:%d, recv txn done
> msg queue full\n", sc->id, chnl);
>         }
>         DEBUG_MSG(KERN_INFO "riffa: fpga:%d chnl:%d, recv txn done\n",
> sc->id, chnl);
> }
> 
> > https://gist.github.com/promach/7716ee8addcaa33fda140d74d1ad94d6#file-riffa_driver-c-L663
> > 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
> 
> if I do not use ->producer in the above multiple (due to multiple if()
> clause) sequential
> push_circ_queue() operations,  what else I can use other than
> ->producer to store the data
> for item_recv_push   ? Probably a simple integer indexing will do.


maybe

> 
> 
> 
> > https://i.imgur.com/xWJOH1G.png
> > 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
> 
> This "ptr_ring packet" drop does not make any sense to me.
> 
> Regards,
> Phung


i suspect that you overwrite an entry in your data structure
due to race wrt producer index access.