netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Question on ptr_ring linux header
@ 2019-01-15  4:33 fei phung
  2019-01-15 12:48 ` Michael S. Tsirkin
  0 siblings, 1 reply; 13+ messages in thread
From: fei phung @ 2019-01-15  4:33 UTC (permalink / raw)
  To: netdev, majordomo, mst, feiphung

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
https://gist.github.com/promach/65e9331d55a43a2815239430a28e29c6#file-circ_ring-c-L44


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;
}



                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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: Question on ptr_ring linux header
@ 2019-01-15 17:10 fei phung
  2019-01-15 18:13 ` Michael S. Tsirkin
  0 siblings, 1 reply; 13+ messages in thread
From: fei phung @ 2019-01-15 17:10 UTC (permalink / raw)
  To: netdev, mst, feiphung

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   ?




// 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.




> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: Question on ptr_ring linux header
@ 2019-01-16  7:00 fei phung
  0 siblings, 0 replies; 13+ messages in thread
From: fei phung @ 2019-01-16  7:00 UTC (permalink / raw)
  To: mst, netdev, feiphung

Hi,

> https://gist.github.com/promach/65e9331d55a43a2815239430a28e29c6#file-circ_ring-c-L62
> racy if there are multiple consumers.
> just call ptr_ring_consume_any.

If I modify pop_circ_queue() below to directly use
ptr_ring_consume_any() without
ptr_ring_empty_any() , I am afraid I am running into system crash due
to NULL pointer
deferencing for *item_pop:

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

Just one silly beginner question, why will ptr_ring_empty_any() lead
to data race problem ?


inline int pop_circ_queue(struct ptr_ring * buffer, struct item * item_pop)
{
        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));

        if((item_pop != NULL) && (item_pop->val1 > 0))
        {
        // val1 will never be zero since the event number starts from
1 (so, val1 in push_circ_queue() will not be zero, same case after
pop_circ_queue()), and 0 is only possible during initialization, not
during pop_circ_queue()

                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 {
                DEBUG_MSG(KERN_INFO "empty, nothing to pop from the ring\n");

                return 1;
        }
}



Regards,
Phung

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: Question on ptr_ring linux header
@ 2019-01-17  3:51 fei phung
  2019-01-31  5:16 ` fei phung
  0 siblings, 1 reply; 13+ messages in thread
From: fei phung @ 2019-01-17  3:51 UTC (permalink / raw)
  To: mst, feiphung, netdev

Hi,

I am having data race from threadsanitizer report.

Could you see if there is still data race in the following code using
ptr_ring API ?



/*
 * Filename: circ_ring.c
 * Version: 1.0
 * Description: A circular buffer using API from
 * https://github.com/torvalds/linux/blob/master/include/linux/ptr_ring.h
 */

#include <linux/slab.h>
#include <linux/mm.h>
#include <linux/ptr_ring.h>
#include "circ_ring.h"
//#include <assert.h>

#define DEBUG 1

#ifdef DEBUG
#define DEBUG_MSG(...) printk(__VA_ARGS__)
#else
#define DEBUG_MSG(...)
#endif

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;
}

inline int push_circ_queue(struct ptr_ring * buffer, struct item * item_push)
{
    /* insert one item into the buffer */
    if(ptr_ring_produce_any(buffer, item_push) == 0) // operation is successful
    {
        DEBUG_MSG(KERN_INFO "Successfully pushed val1 = %u and val2 =
%u\n", item_push->val1, item_push->val2);

        return 0;
    }

    else {
        DEBUG_MSG(KERN_INFO "full, not enough buffer space\n");

        return 1;
    }
}

inline int pop_circ_queue(struct ptr_ring * buffer, struct item * item_pop)
{
    struct item * item_temp = 0;

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

    if(item_temp) // (!= NULL)
    {
        item_pop->val1 = item_temp->val1;
        item_pop->val2 = item_temp->val2;

    // val1 will never be zero since the event number starts from 1
(so, val1 in push_circ_queue() will not be zero, same case after
pop_circ_queue()), and 0 is only possible during initialization, not
during pop_circ_queue()
        //assert(item_pop->val1 != 0);

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

        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 {
        //DEBUG_MSG(KERN_INFO "empty, nothing to pop from the ring\n");

        return 1;
    }
}

void free_circ_queue(struct ptr_ring * q)
{
    ptr_ring_cleanup(q, NULL);
}



Regards,
Phung

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-03-01  3:20 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-15  4:33 Question on ptr_ring linux header fei phung
2019-01-15 12:48 ` Michael S. Tsirkin
  -- strict thread matches above, loose matches on Subject: below --
2019-01-15 17:10 fei phung
2019-01-15 18:13 ` Michael S. Tsirkin
     [not found]   ` <SG2PR06MB3176FA4C12888C84990F109BC1820@SG2PR06MB3176.apcprd06.prod.outlook.com>
2019-01-16 14:09     ` Michael S. Tsirkin
2019-01-16  7:00 fei phung
2019-01-17  3:51 fei phung
2019-01-31  5:16 ` fei phung
2019-01-31 14:39   ` Michael S. Tsirkin
2019-02-01  8:12     ` fei phung
2019-02-01 15:36       ` Michael S. Tsirkin
2019-02-15  3:03         ` fei phung
2019-03-01  3:20           ` fei phung

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).