On 10/2/2018 1:58 PM, Wiles, Keith wrote:
>
>
>> On Oct 2, 2018, at 5:30 AM, Raslan Darawsheh <rasl...@mellanox.com> wrote:
>>
>> Hi,
>>
>> what I'm really doing is simply do some private array for all the fd's that
>> each process will allocate it separately which will allow that each process
>> will be
>> able to access the fd's for the queues in order not to overwrite the shared
>> ones and it's working for me this way.
>>
>> Now coming to your comment I'm not sure I fully understand it but, what you
>> are suggesting is to create an array which will be accessed by the
>> process_id to store these fd's in it.
>> As far as I know we don't have something as process_id in dpdk we only have
>> the system process id which is not relevant for the array of fd's.
>>
>> Please correct me if I'm wrong,
>> I think this way we'll be limiting the number of secondary processes to
>> number of queues by tap.
>> Meanwhile, in my solution we don't have such limitation.
>
> I did not explain myself very well in the previous email let me try again.
>
> Right now you have a pointer to an allocated piece of memory holding the FD’s
> struct pmd_process_private *process_private; if we converted the pointer to
> an array of pmd_process_private structures using RTE_PMD_TAP_MAX_QUEUES as an
> index.
>
> struct pmd_process_private {
> int rx_fd;
> int tx_fd;
> };
>
> static struct pmd_process_private private_process[RTE_PMD_TAP_MAX_QUEUES]; /*
> each process has its own array */
>
> struct rx_queue *rxq = pmd->dev->data->rx_queues[i];
>
> struct rx_queue {
> struct pmd_process_private *priv; // rxq->priv =
> &private_process[rx_queue_idx];
> . . .
> };
>
> rxq->priv->rx_fd; // instead of the *rxq->fd seems shorter but less clear to
> me
>
> Anyway my email compiler is not very good and maybe this will not work.
This should work, it is another kind of redirection.
But I think there are other problems, current implementation seems broken, I
will comment more detailed to the patch.
Briefly, rx_pkt_burst/tx_pkt_burst are not in eth_dev->data, but eth_dev because
they need to be private to process (can't share function pointers across
processes), this is the same problem.
>
> We can leave the current design in place for now, till I have time to really
> look at doing a PoC for my suggestion.
>
>>
>> Kindest regards,
>> Raslan Darawsheh
>>
>> -----Original Message-----
>> From: Wiles, Keith <keith.wi...@intel.com>
>> Sent: Thursday, September 27, 2018 4:18 PM
>> To: Raslan Darawsheh <rasl...@mellanox.com>
>> Cc: Thomas Monjalon <tho...@monjalon.net>; dev@dpdk.org; Shahaf Shuler
>> <shah...@mellanox.com>; Ori Katz <o...@mellanox.com>
>> Subject: Re: [PATCH v3 1/2] net/tap: change queue fd to be pointers to
>> process private
>>
>>
>>
>>> On Sep 27, 2018, at 6:19 AM, Raslan Darawsheh <rasl...@mellanox.com> wrote:
>>>
>>> change the fds for the queues to be pointers and add new process
>>> private structure and make the queue fds point to it.
>>>
>>> Signed-off-by: Raslan Darawsheh <rasl...@mellanox.com>
>>> ---
>>> drivers/net/tap/rte_eth_tap.c | 63
>>> ++++++++++++++++++++++++-------------------
>>> drivers/net/tap/rte_eth_tap.h | 9 +++++--
>>> drivers/net/tap/tap_intr.c | 4 +--
>>> 3 files changed, 44 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c
>>> b/drivers/net/tap/rte_eth_tap.c index ad5ae98..8cc4552 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -64,6 +64,7 @@
>>>
>>> static struct rte_vdev_driver pmd_tap_drv; static struct
>>> rte_vdev_driver pmd_tun_drv;
>>> +static struct pmd_process_private *process_private;
>>
>> Maybe I do not see some minor point for making fd a pointer to fd when we
>> could have an array of process_private[RTE_PMD_TAP_MAX_QUEUES] instead of a
>> pointer type here. Then we do not need to allocate the memory each PMD and
>> they would still have a private copy. Remove the array of rx/tx fds in the
>> structure. This way it appears we can remove the code below that is making
>> fd a pointer to fd. It just seems overly complex to me at the cost of a few
>> more bytes of memory.
>>
>> This would remove int fd; from the structure and add a pointer to the
>> pid_process_private instead, which is private by default.
>>
>> Did I miss some detail here that makes my comment wrong?
>>
>>>
>>> static const char *valid_arguments[] = {
>>> ETH_TAP_IFACE_ARG,
>>> @@ -331,7 +332,7 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs,
>>> uint16_t nb_pkts)
>>> uint16_t data_off = rte_pktmbuf_headroom(mbuf);
>>> int len;
>>>
>>> - len = readv(rxq->fd, *rxq->iovecs,
>>> + len = readv(*rxq->fd, *rxq->iovecs,
>>> 1 +
>>> (rxq->rxmode->offloads & DEV_RX_OFFLOAD_SCATTER ?
>>> rxq->nb_rx_desc : 1));
>>> @@ -595,7 +596,7 @@ tap_write_mbufs(struct tx_queue *txq, uint16_t
>>> num_mbufs,
>>> tap_tx_l4_cksum(l4_cksum, l4_phdr_cksum, l4_raw_cksum);
>>>
>>> /* copy the tx frame data */
>>> - n = writev(txq->fd, iovecs, j);
>>> + n = writev(*txq->fd, iovecs, j);
>>> if (n <= 0)
>>> break;
>>> (*num_packets)++;
>>> @@ -976,13 +977,13 @@ tap_dev_close(struct rte_eth_dev *dev)
>>> tap_flow_implicit_flush(internals, NULL);
>>>
>>> for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>>> - if (internals->rxq[i].fd != -1) {
>>> - close(internals->rxq[i].fd);
>>> - internals->rxq[i].fd = -1;
>>> + if (*internals->rxq[i].fd != -1) {
>>> + close(*internals->rxq[i].fd);
>>> + *internals->rxq[i].fd = -1;
>>> }
>>> - if (internals->txq[i].fd != -1) {
>>> - close(internals->txq[i].fd);
>>> - internals->txq[i].fd = -1;
>>> + if (*internals->txq[i].fd != -1) {
>>> + close(*internals->txq[i].fd);
>>> + *internals->txq[i].fd = -1;
>>> }
>>> }
>>>
>>> @@ -1007,9 +1008,9 @@ tap_rx_queue_release(void *queue) {
>>> struct rx_queue *rxq = queue;
>>>
>>> - if (rxq && (rxq->fd > 0)) {
>>> - close(rxq->fd);
>>> - rxq->fd = -1;
>>> + if (rxq && rxq->fd && (*rxq->fd > 0)) {
>>> + close(*rxq->fd);
>>> + *rxq->fd = -1;
>>> rte_pktmbuf_free(rxq->pool);
>>> rte_free(rxq->iovecs);
>>> rxq->pool = NULL;
>>> @@ -1022,9 +1023,9 @@ tap_tx_queue_release(void *queue) {
>>> struct tx_queue *txq = queue;
>>>
>>> - if (txq && (txq->fd > 0)) {
>>> - close(txq->fd);
>>> - txq->fd = -1;
>>> + if (txq && txq->fd && (*txq->fd > 0)) {
>>> + close(*txq->fd);
>>> + *txq->fd = -1;
>>> }
>>> }
>>>
>>> @@ -1214,13 +1215,13 @@ tap_setup_queue(struct rte_eth_dev *dev,
>>> struct rte_gso_ctx *gso_ctx;
>>>
>>> if (is_rx) {
>>> - fd = &rx->fd;
>>> - other_fd = &tx->fd;
>>> + fd = rx->fd;
>>> + other_fd = tx->fd;
>>> dir = "rx";
>>> gso_ctx = NULL;
>>> } else {
>>> - fd = &tx->fd;
>>> - other_fd = &rx->fd;
>>> + fd = tx->fd;
>>> + other_fd = rx->fd;
>>> dir = "tx";
>>> gso_ctx = &tx->gso_ctx;
>>> }
>>> @@ -1331,7 +1332,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>>> }
>>>
>>> TAP_LOG(DEBUG, " RX TUNTAP device name %s, qid %d on fd %d",
>>> - internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>>> + internals->name, rx_queue_id, *internals->rxq[rx_queue_id].fd);
>>>
>>> return 0;
>>>
>>> @@ -1371,7 +1372,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
>>> return -1;
>>> TAP_LOG(DEBUG,
>>> " TX TUNTAP device name %s, qid %d on fd %d csum %s",
>>> - internals->name, tx_queue_id, internals->txq[tx_queue_id].fd,
>>> + internals->name, tx_queue_id, *internals->txq[tx_queue_id].fd,
>>> txq->csum ? "on" : "off");
>>>
>>> return 0;
>>> @@ -1633,6 +1634,10 @@ eth_dev_tap_create(struct rte_vdev_device *vdev,
>>> char *tap_name,
>>> goto error_exit_nodev;
>>> }
>>>
>>> + process_private = (struct pmd_process_private *)
>>> + rte_zmalloc_socket(tap_name, sizeof(struct pmd_process_private),
>>> + RTE_CACHE_LINE_SIZE, dev->device->numa_node);
>>> +
>>> pmd = dev->data->dev_private;
>>> pmd->dev = dev;
>>> snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name); @@ -1669,8
>>> +1674,10 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
>>> /* Presetup the fds to -1 as being not valid */
>>> pmd->ka_fd = -1;
>>> for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>>> - pmd->rxq[i].fd = -1;
>>> - pmd->txq[i].fd = -1;
>>> + process_private->rxq_fds[i] = -1;
>>> + process_private->txq_fds[i] = -1;
>>> + pmd->rxq[i].fd = &process_private->rxq_fds[i];
>>> + pmd->txq[i].fd = &process_private->txq_fds[i];
>>> }
>>>
>>> if (pmd->type == ETH_TUNTAP_TYPE_TAP) { @@ -2089,13 +2096,13 @@
>>> rte_pmd_tap_remove(struct rte_vdev_device *dev)
>>> tap_nl_final(internals->nlsk_fd);
>>> }
>>> for (i = 0; i < RTE_PMD_TAP_MAX_QUEUES; i++) {
>>> - if (internals->rxq[i].fd != -1) {
>>> - close(internals->rxq[i].fd);
>>> - internals->rxq[i].fd = -1;
>>> + if (*internals->rxq[i].fd != -1) {
>>> + close(*internals->rxq[i].fd);
>>> + *internals->rxq[i].fd = -1;
>>> }
>>> - if (internals->txq[i].fd != -1) {
>>> - close(internals->txq[i].fd);
>>> - internals->txq[i].fd = -1;
>>> + if (*internals->txq[i].fd != -1) {
>>> + close(*internals->txq[i].fd);
>>> + *internals->txq[i].fd = -1;
>>> }
>>> }
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.h
>>> b/drivers/net/tap/rte_eth_tap.h index 44e2773..4146f5c 100644
>>> --- a/drivers/net/tap/rte_eth_tap.h
>>> +++ b/drivers/net/tap/rte_eth_tap.h
>>> @@ -46,7 +46,7 @@ struct rx_queue {
>>> struct rte_mempool *mp; /* Mempool for RX packets */
>>> uint32_t trigger_seen; /* Last seen Rx trigger value */
>>> uint16_t in_port; /* Port ID */
>>> - int fd;
>>> + int *fd;
>>> struct pkt_stats stats; /* Stats for this RX queue */
>>> uint16_t nb_rx_desc; /* max number of mbufs available */
>>> struct rte_eth_rxmode *rxmode; /* RX features */ @@ -56,7 +56,7 @@
>>> struct rx_queue { };
>>>
>>> struct tx_queue {
>>> - int fd;
>>> + int *fd;
>>> int type; /* Type field - TUN|TAP */
>>> uint16_t *mtu; /* Pointer to MTU from dev_data */
>>> uint16_t csum:1; /* Enable checksum offloading */
>>> @@ -92,6 +92,11 @@ struct pmd_internals {
>>> int ka_fd; /* keep-alive file descriptor */
>>> };
>>>
>>> +struct pmd_process_private {
>>> + int rxq_fds[RTE_PMD_TAP_MAX_QUEUES];
>>> + int txq_fds[RTE_PMD_TAP_MAX_QUEUES]; };
>>> +
>>> /* tap_intr.c */
>>>
>>> int tap_rx_intr_vec_set(struct rte_eth_dev *dev, int set); diff --git
>>> a/drivers/net/tap/tap_intr.c b/drivers/net/tap/tap_intr.c index
>>> fc59018..a4b212d 100644
>>> --- a/drivers/net/tap/tap_intr.c
>>> +++ b/drivers/net/tap/tap_intr.c
>>> @@ -71,7 +71,7 @@ tap_rx_intr_vec_install(struct rte_eth_dev *dev)
>>> struct rx_queue *rxq = pmd->dev->data->rx_queues[i];
>>>
>>> /* Skip queues that cannot request interrupts. */
>>> - if (!rxq || rxq->fd <= 0) {
>>> + if (!rxq || !rxq->fd || *rxq->fd <= 0) {
>>> /* Use invalid intr_vec[] index to disable entry. */
>>> intr_handle->intr_vec[i] =
>>> RTE_INTR_VEC_RXTX_OFFSET +
>>> @@ -79,7 +79,7 @@ tap_rx_intr_vec_install(struct rte_eth_dev *dev)
>>> continue;
>>> }
>>> intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + count;
>>> - intr_handle->efds[count] = rxq->fd;
>>> + intr_handle->efds[count] = *rxq->fd;
>>> count++;
>>> }
>>> if (!count)
>>> --
>>> 2.7.4
>>>
>>
>> Regards,
>> Keith
>>
>
> Regards,
> Keith
>
