Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pointer support on Atomic (WIP) #5713

Closed
wants to merge 2 commits into from

Conversation

S-YOU
Copy link

@S-YOU S-YOU commented Feb 12, 2018

No description provided.

@asterite
Copy link
Member

Use the existing constructor, using pointerof like that is wrong, it's a dangling pointer. You just have to change the macro if in the existing constructor to also allow pointers. I can do it this Wednesday.

@S-YOU
Copy link
Author

S-YOU commented Feb 12, 2018

Ok, thank you very much. I still have lack of knowledge on internals. Closing.

@S-YOU S-YOU closed this Feb 12, 2018
@asterite
Copy link
Member

@S-YOU what do you want do do? Why do you need a pointer? I'm pretty sure Atomic is already useful for your use case.

@S-YOU
Copy link
Author

S-YOU commented Feb 12, 2018

volatile struct app_stats {
	struct {
		uint64_t rx_pkts;
		uint64_t enqueue_pkts;
		uint64_t enqueue_failed_pkts;
	} rx __rte_cache_aligned;

	struct {
		uint64_t dequeue_pkts;
		uint64_t enqueue_pkts;
		uint64_t enqueue_failed_pkts;
	} wkr __rte_cache_aligned;

	struct {
		uint64_t dequeue_pkts;
		/* Too early pkts transmitted directly w/o reordering */
		uint64_t early_pkts_txtd_woro;
		/* Too early pkts failed from direct transmit */
		uint64_t early_pkts_tx_failed_woro;
		uint64_t ro_tx_pkts;
		uint64_t ro_tx_failed_pkts;
	} tx __rte_cache_aligned;
} app_stats;

....
while (!quit_signal) {

		/* dequeue the mbufs from rx_to_workers ring */
		burst_size = rte_ring_dequeue_burst(ring_in,
				(void *)burst_buffer, MAX_PKTS_BURST, NULL);
		if (unlikely(burst_size == 0))
			continue;

		__sync_fetch_and_add(&app_stats.wkr.dequeue_pkts, burst_size);

		/* just do some operation on mbuf */
		for (i = 0; i < burst_size;)
			burst_buffer[i++]->port ^= xor_val;

		/* enqueue the modified mbufs to workers_to_tx ring */
		ret = rte_ring_enqueue_burst(ring_out, (void *)burst_buffer,
				burst_size, NULL);
		__sync_fetch_and_add(&app_stats.wkr.enqueue_pkts, ret);
		if (unlikely(ret < burst_size)) {
			/* Return the mbufs to their respective pool, dropping packets */
			__sync_fetch_and_add(&app_stats.wkr.enqueue_failed_pkts,
					(int)burst_size - ret);
			pktmbuf_free_bulk(&burst_buffer[ret], burst_size - ret);
		}
	}

@asterite, I would like to convert above C code to Crystal. equivalent of __sync_fetch_and_add is only on Atomic as far as I understand. It is also under multi-threaded environment, but not really writing the same variable at the same time. EDIT: worker threads can be spawn as many as CPU available, so, it could be writing to same variable from many threads, at the same time.

@S-YOU S-YOU deleted the atomic_pointer branch February 12, 2018 16:49
@asterite
Copy link
Member

I'm not sure you can write such code in Crystal. You want an atomic int. Maybe better to use Rust for this kind of code.

@S-YOU
Copy link
Author

S-YOU commented Feb 13, 2018

Well, rust is too much verbose for me. The approach I was trying with AtomicPointer pointer on another thread seems to work, but no idea how to mitigate dangling pointer issue.

@S-YOU
Copy link
Author

S-YOU commented Feb 13, 2018

Also I see the potential on Crystal for C/C++ programmers, not only Ruby programmers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants